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