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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
