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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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? Likewise, instead of `addDependencies`, perhaps `addDependence`?



================
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 commonly used combinations.
----------------
Hmm. We have a different set of propagated flags for types (dependent / instantiation dependent / unexpanded pack / variably-modified) and for (eg) template arguments and nested name specifiers (dependent / instantiation dependent / unexpanded pack). It would be nice to use the same machinery everywhere without introducing the possibility of meaningless states and giving the wrong names to some states.

I think we should aim for something more type-safe than this: use a different type for each different family of AST nodes, so we don't conflate "dependent" for template arguments with one or both of "type-dependent" and "value-dependent" for expressions, which mean different things.


================
Comment at: clang/include/clang/AST/DependencyFlags.h:32-59
+constexpr inline DependencyFlags operator~(DependencyFlags F) {
+  return static_cast<DependencyFlags>(
+      ~static_cast<unsigned>(F) & static_cast<unsigned>(DependencyFlags::All));
+}
+
+constexpr inline DependencyFlags operator|(DependencyFlags L,
+                                           DependencyFlags R) {
----------------
You can use LLVM's `BitmaskEnum` mechanism in place of these operators. (`#include "clang/Basic/BitmaskEnum.h"` and add `LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/UnexpandedPack)` to the end of the enum.)


================
Comment at: clang/include/clang/AST/DependencyFlags.h:81-85
+inline DependencyFlags turnTypeToValueDependency(DependencyFlags F) {
+  if (!isTypeDependent(F))
+    return F;
+  return (F & ~DependencyFlags::Type) | DependencyFlags::Value;
+}
----------------
This whole function should be equivalent to just `F & ~DependencyFlags::Type`. Any type-dependent expression must also be value-dependent, so you should never need to set the `::Value` bit. Perhaps we could assert this somewhere.


================
Comment at: clang/lib/AST/NestedNameSpecifier.cpp:221
       if (Base.getType()->isDependentType())
-        return true;
+        return DependencyFlags::Type;
 
----------------
This is wrong: `super` should be instantiation-dependent whenever `Specifier` names a dependent type. Please add a FIXME.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920