vsapsai added a comment.
Thanks for the review, Dan!
================
Comment at: clang/include/clang/AST/Decl.h:1383-1398
/// If this definition should pretend to be a declaration.
bool isThisDeclarationADemotedDefinition() const {
return isa<ParmVarDecl>(this) ? false :
NonParmVarDeclBits.IsThisDeclarationADemotedDefinition;
}
/// This is a definition which should be demoted to a declaration.
----------------
Naming for my change is mimicking this API.
================
Comment at: clang/include/clang/AST/Decl.h:3494
+ /// symbol and not interested in the final AST with deduplicated definitions.
+ bool isThisDeclarationADemotedDefinition() const {
+ return TagDeclBits.IsThisDeclarationADemotedDefinition;
----------------
delcypher wrote:
> This name feels a little clunky. How about `isDemotedDefinition()`?
I agree the name is awkward but I am using the same name as `NonParmVarDecl`
does. Based on similarities in implementation I think the similarities in the
name are useful. Also we have `isThisDeclarationADefinition` in multiple
places, so it is good to follow the same naming pattern, even if it is clunky.
================
Comment at: clang/include/clang/AST/Decl.h:3500
+ /// a definition.
+ void demoteThisDefinitionToDeclaration() {
+ assert(isCompleteDefinition() && "Not a definition!");
----------------
delcypher wrote:
> delcypher wrote:
> > Given the name of this function (suggesting it always `demotes`) it
> > probably should
> >
> > ```
> > assert(!isThisDeclarationADemotedDefinition() && "decl already demoted");
> > ```
> >
> > alternatively comments saying the operation is idempotent is probably fine
> > too.
> The `demoteThisDefinitionToDeclaration()` name feels a little bit clunky. How
> about `demoteDefinitionToDecl()`?
Interesting suggestion, made me think about what exactly I'm trying to do here.
The strict requirement is not to set the flag on forward declarations (aka
non-definitions). To achieve that the alternative would be
```lang=c++
if (!isCompleteDefinition())
return;
setCompleteDefinition(false);
TagDeclBits.IsThisDeclarationADemotedDefinition = true;
```
Which is similar to the current code with the assertion. Demoting already
demoted definition again is acceptable but there is really no need for that.
I agree the assertion makes the code less robust but its purpose is to enforce
expected API usage, so being picky about the way it is called is intended.
Though let me change the assertion message to something more actionable rather
than state-of-the-world-observation.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118855/new/
https://reviews.llvm.org/D118855
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
