This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Include "final" when printing class declaration
ClosedPublic

Authored by tom-anders on Jun 20 2022, 7:05 AM.

Diff Detail

Event Timeline

tom-anders created this revision.Jun 20 2022, 7:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2022, 7:05 AM
tom-anders requested review of this revision.Jun 20 2022, 7:05 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 20 2022, 7:05 AM
sammccall added inline comments.Jun 20 2022, 7:11 AM
clang/lib/AST/DeclPrinter.cpp
1010

isEffectivelyFinal returns true for
struct X { ~X() final; }

I don't think we want to print struct X final {} in that case.

Probably want to replicate the check for FinalAttr instead?

tom-anders added inline comments.Jun 20 2022, 7:17 AM
clang/lib/AST/DeclPrinter.cpp
1010

Agreed. Would probably make sense to add a bool hasFinalAttribute() const to CXXRecordDecl?

sammccall added inline comments.Jun 21 2022, 5:41 AM
clang/lib/AST/DeclPrinter.cpp
1010

I'm not sure such a method pays for itself, the abstraction it might provides over hasAttr() is:

  • it handles the case where there's no definition... but maybe "return false" isn't the right thing to do in all circumstances?
  • a simple hasFinal() could avoid thinking about syntactic details - but here we explicitly care about them.
  • it hides the detail that final is stored as an attribute rather than e.g. a bit. This is the only piece that seems useful.

Given that, I think it's probably best here just to inline the logic, but I don't feel strongly.

Check for FinalAttr instead of using isEffectivelyFinal()

sammccall accepted this revision.Jun 28 2022, 10:08 AM
This revision is now accepted and ready to land.Jun 28 2022, 10:08 AM
This revision was landed with ongoing or failed builds.Jul 11 2022, 3:22 AM
This revision was automatically updated to reflect the committed changes.