[PATCH] D118855: [modules] Add a flag for TagDecl if it was a definition demoted to a declaration.

Volodymyr Sapsai via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Feb 14 11:46:17 PST 2022


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



More information about the cfe-commits mailing list