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

[Ilya Biryukov via Phabricator via cfe-commits]

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 relevant comment.

Other than that, this should be ready for the next round.

In D71920#1843488 <https://reviews.llvm.org/D71920#1843488>, @rsmith wrote:

> 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`?


Good point. I've opted for `getDependence`, but didn't have enough time today to change `addDependencies`, will make sure to do it in the next iteration.



================
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.
----------------
ilya-biryukov wrote:
> 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.
I've introduced a separate enum for each of the AST classes, but that looks like an overkill.
We could probably merge the three for template arguments, template names and nested name specifiers into one.
(Would allow to get rid of macros too).

Note that `TypeDependence` is missing variably-modified bit, was't sure if it's part of the dependency propagation (don't have enough context to understand what it does and didn't look into it). From you message, I assume you would prefer to have it in the enum too?

WDYT about the approach in general?


================
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) {
----------------
rsmith wrote:
> 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.)
Thanks, that does look better. I've used this and also switched to non-strongly-typed enums to allow simple conversions to bool (and code like `bool IsTypeDependent = D & ExprDependence::Type`).

There's a catch, though. We can't use `LLVM_MARK_AS_BITMASK_ENUM` on two enums inside the same namespace, it results in redeclaration of the same enumerator.

So I had to put them into structs (to introduce a scope). Might be a bit weird, let me know what you think, changing back to strongly-typed enums should not be too hard. That would mean we need helpers like `isTypeDependent` or checks like `(D & Flags::Type) != Flags::None`, both don't look good.


================
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;
+}
----------------
ilya-biryukov wrote:
> 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.
Was thinking about the best place for assert.
Putting it into headers means introducing possibly ODR violations, we do care about keeping those off LLVM, right?
Putting it into the .cpp files means we're loosing optimization opportunities in non-LTO builds (those functions should really be inlined).

Decided to not put the assertions here for now, but those could be easy to add later.


================
Comment at: clang/lib/AST/NestedNameSpecifier.cpp:221
       if (Base.getType()->isDependentType())
-        return true;
+        return DependencyFlags::Type;
 
----------------
rsmith wrote:
> This is wrong: `super` should be instantiation-dependent whenever `Specifier` names a dependent type. Please add a FIXME.
Done.
This is true for everything, right? Dependent always implies instantiation-dependent, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920