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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
