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

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

Change seems reasonable but I don't have expertise on this code. I've left a few minor nits.



================
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;
----------------
This name feels a little clunky. How about `isDemotedDefinition()`?


================
Comment at: clang/include/clang/AST/Decl.h:3500
+  /// a definition.
+  void demoteThisDefinitionToDeclaration() {
+    assert(isCompleteDefinition() && "Not a definition!");
----------------
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.


================
Comment at: clang/include/clang/AST/Decl.h:3500
+  /// a definition.
+  void demoteThisDefinitionToDeclaration() {
+    assert(isCompleteDefinition() && "Not a definition!");
----------------
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()`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118855/new/

https://reviews.llvm.org/D118855