[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

AlexeySachkov added inline comments. Comment at: clang/include/clang/AST/Type.h:1827-1830 +if (Dependent) + Deps |= TypeDependence::Dependent | TypeDependence::Instantiation; +if (InstantiationDependent) + Deps |= TypeDependence::Instantiation;

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

sammccall added inline comments. Comment at: clang/include/clang/AST/Type.h:1827-1830 +if (Dependent) + Deps |= TypeDependence::Dependent | TypeDependence::Instantiation; +if (InstantiationDependent) + Deps |= TypeDependence::Instantiation;

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

AlexeySachkov added inline comments. Comment at: clang/include/clang/AST/Type.h:1827-1830 +if (Dependent) + Deps |= TypeDependence::Dependent | TypeDependence::Instantiation; +if (InstantiationDependent) + Deps |= TypeDependence::Instantiation;

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

This revision was automatically updated to reflect the committed changes. hokein marked an inline comment as done. Closed by commit rGec3060c72de6: [AST] Refactor propagation of dependency bits. NFC (authored by ilya-biryukov, committed by hokein). Changed prior to commit:

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

sammccall added inline comments. Comment at: clang/lib/AST/TemplateName.cpp:208 "overloaded templates shouldn't survive to here"); + D |= TemplateNameDependence::DependentInstantiation; + return D; hokein wrote: > sammccall wrote: > > what's this

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

hokein added inline comments. Comment at: clang/lib/AST/TemplateName.cpp:208 "overloaded templates shouldn't survive to here"); + D |= TemplateNameDependence::DependentInstantiation; + return D; sammccall wrote: > what's this line about? this

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

hokein updated this revision to Diff 247905. hokein marked an inline comment as done. hokein added a comment. move the assert to overloadedTemplate switch case. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71920/new/

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. thanks, that looks better to me Comment at: clang/lib/AST/TemplateName.cpp:193 + } else { +assert(!getAsOverloadedTemplate() && + "overloaded templates

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

hokein updated this revision to Diff 247881. hokein marked 4 inline comments as done. hokein added a comment. address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71920/new/ https://reviews.llvm.org/D71920 Files:

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

hokein added inline comments. Comment at: clang/lib/AST/TemplateName.cpp:193 + } else { +assert(!getAsOverloadedTemplate() && + "overloaded templates shouldn't survive to here"); sammccall wrote: > As far as I can tell, an overloaded template will

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

sammccall added inline comments. Comment at: clang/lib/AST/TemplateName.cpp:174 +TemplateNameDependence TemplateName::getDependence() const { + auto Dependency = TemplateNameDependence::None; + if (QualifiedTemplateName *QTN = getAsQualifiedTemplateName())

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

hokein updated this revision to Diff 247818. hokein added a comment. - rebase to master - address my comments - added a FIXME about the RequiresExpr Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71920/new/ https://reviews.llvm.org/D71920 Files:

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

hokein commandeered this revision. hokein edited reviewers, added: sammccall; removed: hokein. hokein added inline comments. Comment at: clang/lib/AST/TemplateName.cpp:173 +TemplateNameDependence TemplateName::getDependence() const { + auto F = TemplateNameDependence::None; +

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

hokein added inline comments. Comment at: clang/include/clang/AST/DependencyFlags.h:57 + struct NAME##Scope { \ +enum NAME { \ + UnexpandedPack = 1,

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

sammccall marked an inline comment as done. sammccall added a comment. Thanks Richard! Haojian agreed to take a look at this. Comment at: clang/lib/AST/ExprConcepts.cpp:186 + if (Dependent) +addDependence(ExprDependence::ValueInstantiation); } Per

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

rsmith added a comment. In D71920#1870195 , @sammccall wrote: > If you'd like to have `concept::Requirement` use a similar bitfield, I'd just > like to confirm my understanding of the current code before refactoring it: > > - there's just one `Dependent`

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

rsmith added a comment. In D71920#1884740 , @sammccall wrote: > @rsmith Friendly ping. Do you want to review this/other patches in this > sequence, or should we be a bit bolder and pull you in when we're uncertain > in principle what to do? I'm fine

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

sammccall added a comment. @rsmith Friendly ping. Do you want to review this/other patches in this sequence, or should we be a bit bolder and pull you in when we're uncertain in principle what to do? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

sammccall commandeered this revision. sammccall added a reviewer: ilya-biryukov. sammccall added a comment. @ilya-biryukov isn't working on clang fulltime anymore :-( I'm going to try to pick up the work on RecoveryExpr, starting with this patch. @rsmith I think this is waiting on your review

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

sammccall updated this revision to Diff 243915. sammccall added a comment. Rebase (ConceptSpecializationExpr moved). Added FIXME to use enum for concept requirement dependence, it's still murky to me. addDependencies -> addDependence, for clarity. Move variably-modified into TypeDependence

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

merge_guards_bot added a comment. {icon times-circle color=red} Unit tests: fail. 61841 tests passed, 5 failed and 780 were skipped. failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

ilya-biryukov added a comment. I've opted for duplicating the common flags across all the introduced enums (contains-unexpanded-pack, instantiation-dependent) , this is somewhat ugly, but everything else is even more complicated to use. Less enums would also be a good idea probably, see the

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

ilya-biryukov updated this revision to Diff 240958. ilya-biryukov marked an inline comment as done and an inline comment as not done. ilya-biryukov added a comment. - Use different types for different AST categories - Rename to getDependence Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang/include/clang/AST/DependencyFlags.h:17-28 +enum class DependencyFlags : uint8_t { + Type = 1, + Value = 2, + Instantiation = 4, + UnexpandedPack = 8, + + // Shorthands for

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

rsmith added a comment. I don't like the name `getDependencies`, because the function is not getting a list of dependencies, it's getting flags that indicate whether certain properties of the construct are dependent. Maybe `getDependence` or `getDependenceFlags` would be a better name?

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

merge_guards_bot added a comment. {icon times-circle color=red} Unit tests: fail. 61841 tests passed, 5 failed and 780 were skipped. failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

ilya-biryukov updated this revision to Diff 240604. ilya-biryukov added a comment. - Use DependencyFlags for TemplateName and NestedNameSpecifier Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71920/new/ https://reviews.llvm.org/D71920 Files:

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 61745 tests passed, 0 failed and 780 were skipped. {icon times-circle color=red} clang-tidy: fail. Please fix clang-tidy findings

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

ilya-biryukov updated this revision to Diff 237577. ilya-biryukov added a comment. - Add compound assignment operations Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71920/new/ https://reviews.llvm.org/D71920 Files:

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

ilya-biryukov added a comment. @rsmith, gentle ping. WDYT? Is this a step in the right direction? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71920/new/ https://reviews.llvm.org/D71920 ___

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

ilya-biryukov marked 4 inline comments as done. ilya-biryukov added inline comments. Comment at: clang/include/clang/AST/Expr.h:126 +if (TD) + D = D | DependencyFlags::Type; +if (VD) riccibruno wrote: > ilya-biryukov wrote: > > Mordante wrote: > > >

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

riccibruno added inline comments. Comment at: clang/include/clang/AST/Expr.h:126 +if (TD) + D = D | DependencyFlags::Type; +if (VD) ilya-biryukov wrote: > Mordante wrote: > > Just curious why do you prefer `D = D | DependencyFlags::Type;` over `D |=

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 61158 tests passed, 0 failed and 728 were skipped. {icon check-circle color=green} clang-tidy: pass. {icon check-circle color=green} clang-format: pass. Build artifacts

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

ilya-biryukov marked 4 inline comments as done. ilya-biryukov added inline comments. Comment at: clang/include/clang/AST/Expr.h:126 +if (TD) + D = D | DependencyFlags::Type; +if (VD) Mordante wrote: > Just curious why do you prefer `D = D |

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

ilya-biryukov updated this revision to Diff 235837. ilya-biryukov added a comment. - Use DependencyFlagsBits for computing NumExprBits - Reformat Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71920/new/ https://reviews.llvm.org/D71920 Files:

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

Mordante added a comment. I like the goal of this patch and the simplifications it does. I don't feel qualified to do a full review. Comment at: clang/include/clang/AST/Expr.h:126 +if (TD) + D = D | DependencyFlags::Type; +if (VD) Just curious

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 61128 tests passed, 0 failed and 728 were skipped. {icon check-circle color=green} clang-tidy: pass. {icon times-circle color=red} clang-format: fail. Please format your changes with clang-format by running

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clang-tidy/hicpp/ExceptionBaseclassCheck.cpp:17 +using namespace clang::ast_matchers; + NOTE: We need this to fix compiler errors down below.

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

ilya-biryukov added a comment. Also note that this change does not move any code around to make sure this change is easy to review and validate that the code is doing the same thing. I'm also planning to move all the code that computes dependencies into one place (it's currently scattered

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

ilya-biryukov added a comment. Herald added a subscriber: rnkovacs. @rsmith, could you please take a look and let me know whether you think adding a new type for this makes sense? On one hand, I feel it's good to have a type to represent "dependencies" and it allow to write helper functions

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

ilya-biryukov created this revision. ilya-biryukov added a reviewer: rsmith. Herald added a reviewer: martong. Herald added a reviewer: shafik. Herald added a project: clang. This changes introduces an enum to represent dependencies as a bitmask and extract common patterns from code that computes