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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/lib/AST/TemplateName.cpp:174
+TemplateNameDependence TemplateName::getDependence() const {
+  auto Dependency = TemplateNameDependence::None;
+  if (QualifiedTemplateName *QTN = getAsQualifiedTemplateName())
----------------
nit: please don't use "Dependency" to mean "Dependence" :-)

I find this easier to read with a short name here (like `D` or the previous `F`, but up to you.


================
Comment at: clang/lib/AST/TemplateName.cpp:175
+  auto Dependency = TemplateNameDependence::None;
+  if (QualifiedTemplateName *QTN = getAsQualifiedTemplateName())
+    Dependency |=
----------------
I think it would add some useful structure to put everything except the getAsTemplateDecl() in a `switch (getKind())` rather than ifs scattered around. This would make it clearer which cases are disjoint.


================
Comment at: clang/lib/AST/TemplateName.cpp:193
+  } else {
+    assert(!getAsOverloadedTemplate() &&
+           "overloaded templates shouldn't survive to here");
----------------
As far as I can tell, an overloaded template will always return null for getAsTemplateDecl(). So this assert can be hoisted to the top, which I think would be clearer.


================
Comment at: clang/lib/AST/TemplateName.cpp:201
+        DTN->getQualifier()->containsUnexpandedParameterPack())
+      Dependency |= TemplateNameDependence::UnexpandedPack;
+  if (getAsSubstTemplateTemplateParmPack())
----------------
Why are we querying/propagating individual bits here rather than converting and adding DTN->getQualifier()->getDependence()? Are we deliberately dropping some of the bits?

(Checking individual bits makes adding new bits e.g. errors harder, as they won't be propagated by default)


================
Comment at: clang/lib/AST/TemplateName.cpp:205
+  // Dependent template name is always instantiation dependent.
+  if (Dependency & TemplateNameDependence::Dependent)
+    Dependency |= TemplateNameDependence::DependentInstantiation;
----------------
why not just add DependentInstantiation above in the first place?
Tracking incorrect bits and then fixing them at the end seems a bit confusing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920