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

[Ilya Biryukov via Phabricator via cfe-commits]

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 commonly used combinations.
----------------
rsmith wrote:
> 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.
Yep, sounds good, probably the types would end up being more complicated, but I also like more type-safe approach.
Let me try to come up with something, I'll send a patch today.


================
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;
+}
----------------
rsmith wrote:
> 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.
Good point, I'll try to find a place to assert this with multiple types for different AST categories.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920