sammccall added a comment.
(There are still outstanding comments e.g. in ASTImport)
I think it would be useful to add to the patch description:
- the current deficiencies of ConceptReference that make it not a well-behaved
AST node now
- which of these are addressed in this patch (RecursiveASTVisitor, use in all
relevant places, avoiding multiple inheritance)
- which of these will be addressed in future patches (DynTypedNode, maybe
text-dumper?)
================
Comment at: clang/include/clang/AST/ASTConcept.h:112
/// \brief Common data class for constructs that reference concepts with
/// template arguments.
----------------
Doc comment is out of date. Suggestion:
```
/// A reference to a concept and its template args, as it appears in the code.
///
/// Examples:
/// template <int X> requires is_even<X> int half = X/2;
/// ~~~~~~~~~~ (in ConceptSpecializationExpr)
///
/// std::input_iterator auto I = Container.begin();
/// ~~~~~~~~~~~~~~~~~~~ (in AutoTypeLoc)
///
/// template <std::derives_from<Expr> T> void dump();
/// ~~~~~~~~~~~~~~~~~~~~~~~ (in TemplateTypeParmDecl)
```
================
Comment at: clang/include/clang/AST/ASTConcept.h:140
public:
ConceptReference(NestedNameSpecifierLoc NNS, SourceLocation TemplateKWLoc,
DeclarationNameInfo ConceptNameInfo, NamedDecl *FoundDecl,
----------------
this constructor should now be private, like other AST nodes
================
Comment at: clang/include/clang/AST/ASTConcept.h:148
ConceptReference()
: FoundDecl(nullptr), NamedConcept(nullptr), ArgsAsWritten(nullptr) {}
----------------
following the pattern for other AST nodes, this constructor should be private
or gone. If deserialization needs to create an empty ConceptReference, provide
a CreateEmpty() factory
================
Comment at: clang/include/clang/AST/ASTConcept.h:193
-class TypeConstraint : public ConceptReference {
+class TypeConstraint {
/// \brief The immediately-declared constraint expression introduced by this
----------------
We're not really changing this class, but I think it could use a doc comment as
its relationship to ConceptReference, TemplateTypeParmType, and AutoTypeLoc
wasn't all that clear. Suggestion:
```
/// Models the abbreviated syntax to constrain a template type parameter:
/// template <convertible_to<string> T> void print(T object);
/// ~~~~~~~~~~~~~~~~~~~~~~
/// Semantically, this adds an "immediately-declared constraint" with extra arg:
/// requires convertible_to<T, string>
///
/// In the C++ grammar, a type-constraint is also used for auto types:
/// convertible_to<string> auto X = ...;
/// We do *not* model these as TypeConstraints, but AutoType(Loc) directly.
```
================
Comment at: clang/include/clang/AST/ExprConcepts.h:90
+ // NOTE(massberg): For the first minimal prototype we keep the
+ // following functions to prevent. Later these functions should
+ // be accessed getConceptReference().
----------------
incomplete comment: "to prevent" what?
Also FWIW I don't think *all* uses should be migrated, e.g. `getNamedConcept()`
is fundamental enough to be exposed here even if it's implemented via
ConceptReference.
================
Comment at: clang/include/clang/AST/ExprConcepts.h:140-143
SourceLocation getBeginLoc() const LLVM_READONLY {
- if (auto QualifierLoc = getNestedNameSpecifierLoc())
+ if (auto QualifierLoc = CR->getNestedNameSpecifierLoc())
return QualifierLoc.getBeginLoc();
+ return CR->getConceptNameInfo().getBeginLoc();
----------------
cor3ntin wrote:
> Should there be a getLocation() method on `ConceptReference` ? It seems
> useful. maybe even a `getSourceRange()` method too
agree, though we actually shouldn't call ConceptReference::getLocation() method
here because primary location != start location.
I think
- ConceptReference::getLocation() should return getConceptNameLoc() (just an
alias)
- ConceptSpecializationExpr::getExprLoc() should return
ConceptReference::getLocation()
- ConceptReference::getBeginLoc() should contain this logic,
ConceptSpecializationExpr::getBeginLoc() should return it
- ditto end loc
- ConceptReference::getSourceRange() is a useful helper
- ConceptSpecializationExpr already has getSourceRange() inherited from Stmt
================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:2517
+ const ConceptReference *CR) {
+ TRY_TO(VisitConceptReference(CR));
+ TRY_TO(TraverseNestedNameSpecifierLoc(CR->getNestedNameSpecifierLoc()));
----------------
I think this should be guarded by `if
(!getDerived().shouldTraversePostOrder())`, and then the opposite at the end
================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:318
+ /// \returns false if the visitation was terminated early, true otherwise.
+ bool TraverseConceptLoc(const ConceptLoc *CL);
+
----------------
massberg wrote:
> This is basically the old `TraverseConceptReferenceHelper` function. Is it by
> purpose that this isn't const in most other functions? Could the passes
> objects be changed in such functions?
It's somewhere between intended and awkward historical baggage: most RAVs don't
mutate the nodes, but some do.
I suggest making this take a mutable pointer (rather than the more natural
const reference) purely for consistency - RAV is confusing enough as it is.
================
Comment at: clang/include/clang/AST/TypeLoc.h:2111
+ ConceptLoc *CL = nullptr;
// Followed by a TemplateArgumentLocInfo[]
----------------
hokein wrote:
> just an idea, instead storing a `ConceptLoc*` in `AutoTypeLoc`, there is
> another approach (not sure it was considered before), which is to have a
> `TypeConstraint*` in the `AutoType` (and remove the `ConceptDecl*`).
>
> This gives us a model following the grammar closely. type-constrains is used
> in the auto grammar rule, so it seems reasonable to have `TypeConstraint` in
> `AutoType`. The downside is that there is a lot of indirections: `AutoTypeLoc
> -> AutoType -> TypeConstraint -> ConceptLoc` vs `AutoTypeLoc -> ConceptLoc`.
>
I did look at this. Maybe it's a good idea, but it's not simple and it's mostly
independent of the change in this patch.
The thing that TypeConstraint concretely adds to ConceptReference is the
immediately-declared-constraint.
Today this constrains the TypeParmType and semantic analysis can treat it as a
separate constraint: `void foo(Iterator auto x)` is equivalent to
`template<class T> void foo(T x) requires Iterator<T>` or so.
However this formulation isn't convenient for placeholder types: `Iterator auto
x = ...` isn't a template, there's no code processing requirements where we can
tack on `requires Iterator<auto>`. It also doesn't clearly represent the idea
that the two `auto` placeholders should be the same.
I agree it would be nice to follow the grammar (or if not, use different names)
but I think that it's a separate change, and going from
`AutoTypeLoc{ConceptNameLoc etc}` to
`AutoTypeLoc{ConceptReference{ConceptNameLocEtc}}` is a useful stepping-stone
to `AutoTypeLoc{TypeConstraint{ConceptReference{ConceptNameLoc etc}}}`
================
Comment at: clang/include/clang/AST/TypeLoc.h:2140
+ const NestedNameSpecifierLoc getNestedNameSpecifierLoc() const {
+ if (getConceptLoc())
+ return getConceptLoc()->getNestedNameSpecifierLoc();
----------------
hokein wrote:
> nit: `if (const auto* Concept = getConcetLoc())`, the same for the other
> places.
>
> any reason why we need this null sanity check?
Agree with the style change.
We need the check because there may be no constraining concept here: `auto x =
2` should return an empty NNSLoc, not crash
================
Comment at: clang/include/clang/AST/TypeLoc.h:2212-2213
+ // ConceptLoc.
return TemplateArgumentLoc(getTypePtr()->getTypeConstraintArguments()[i],
- getArgLocInfo(i));
+ getArgInfos()[i]);
}
----------------
massberg wrote:
> I don't know why we need this here.
> `getArgLoc` should only be called if there are constraints and in that case
> `getConceptLoc()` should be set.
> However, we still run into this return. Note that `ArgsInfo` is only
> initialized in `initializeLocal` but never changed afterwards.
I think this FIXME should be addressed before landing this patch.
Currently the code (apparently) stores two copies of the args: one as trailing
objects here and one in the conceptreference.
Ideally we'd remove the trailing objects immediately. A FIXME to do so later is
fine if needed, but if they're inconsistent then we know one of them has a bug,
or we don't really understand the design.
These can lead to downstream bugs now, and also make it less likely such a
FIXME will ever be cleaned up.
================
Comment at: clang/lib/Sema/TreeTransform.h:6856
+ TL.getTypePtr()->getTypeConstraintConcept()->getDeclName());
+ /*DNI.setLoc(TL.getConceptNameLoc());
+ DNI.setName(TL.getTypePtr()->getTypeConstraintConcept()->getDeclName());
----------------
fix/remove commented-out code?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155858/new/
https://reviews.llvm.org/D155858
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
