[PATCH] D155858: Add a concept AST node.
Jens Massberg via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 2 03:53:43 PDT 2023
massberg created this revision.
massberg added a reviewer: sammccall.
Herald added subscribers: ChuanqiXu, martong.
Herald added a reviewer: shafik.
Herald added a project: All.
massberg updated this revision to Diff 544722.
massberg added a comment.
massberg updated this revision to Diff 545601.
massberg updated this revision to Diff 545640.
massberg marked 2 inline comments as done.
massberg updated this revision to Diff 545686.
massberg updated this revision to Diff 545690.
massberg updated this revision to Diff 546037.
massberg updated this revision to Diff 546040.
massberg updated this revision to Diff 546054.
massberg retitled this revision from "Prototype for concept AST nodes." to "Add a concept AST node.".
massberg edited the summary of this revision.
Herald added a subscriber: kadircet.
massberg added reviewers: erichkeane, hokein.
massberg retitled this revision from "Add a concept AST node." to "Add a concept AST node.".
massberg updated this revision to Diff 546094.
massberg updated this revision to Diff 546339.
massberg marked an inline comment as done.
massberg marked an inline comment as done.
massberg edited the summary of this revision.
massberg edited the summary of this revision.
massberg published this revision for review.
Herald added subscribers: cfe-commits, ilya-biryukov.
Herald added a project: clang.
AutoTypeLoc now uses ConceptLoc to store information of a concept instead of storing all information itself.
massberg added a comment.
Initial changes to RecursiveASTVisitor to traverse ConceptLocs.
massberg added a comment.
Add `VisitConceptLoc`.
massberg added a comment.
Add `readConceptLoc` and `AddConceptLoc` functions.
massberg added a comment.
Code clean-up.
massberg added a comment.
Remove `LAngleLoc` and `RAngleLoc` from `AutoTypeLocInfo` and access thse location directly from the template args at thestored `ConceptLoc`.
massberg added a comment.
Fix typo.
massberg added a comment.
Some code simplification.
massberg added a comment.
@erichkeane Hi Erich, we would like to add an AST node for concepts (see description above) and we are interested in your opinion on it.
This patch is a draft of what the new AST node could look like and how it would be part of the existing infrastructure.
massberg added a comment.
Create a patch from draft version.
massberg added a comment.
Updated comments.
hokein added a comment.
Thanks, this looks like in a good sharp, I left some comments.
I think it would be nice to get some high-level design input from @erichkeane before making further changes.
massberg added a comment.
A non-draft version of this patch is in https://reviews.llvm.org/D156867. Future changes will be done there.
(Sorry, but I wasn't able to move this patch from draft state to a regular patch).
================
Comment at: clang/include/clang/AST/ASTConcept.h:115
+It is a reference to a conecpt and it template arguments.
+*/
/// \brief Common data class for constructs that reference concepts with
----------------
It would be nice to update this comment (for the other related classes, `TypeConstraint`), this is an important comment which helps reviewers easier understand the design principles of this change.
================
Comment at: clang/include/clang/AST/ASTConcept.h:118
/// template arguments.
-class ConceptReference {
+class ConceptLoc {
protected:
----------------
I'm not sure the `ConceptLoc` is a reasonable name for this structure.
1) In general, classes named `*Loc` in clang AST are tiny, and they are passed by value, so seeing the usage `ConceptLoc *` in the code is a bit wired and inconsistent.
2) Unlike other `TemplateArgumentLoc`, `NestedNameSpecifierLoc`, `TypeLoc` (where they all have TemplateArgument/TemplateArgumentLoc split), and we don't have a corresponding split for `ConceptLoc`.
I think 2) is probably fine, but 1) is a concern, we plan to make it a real AST node, so we will likely add it to the [DynTypedNode](https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/ASTTypeTraits.h#L508), adding a big class there will increase the `DynTypedNode` a lot (from 40 bytes => 80 bytes).
One idea is to make this class `ConceptLocInfo`, and create a new wrapper class `ConceptLoc` wrapper which contains a `ConceptLocInfo* pointer`, and a `ConceptDecl* concept`.
================
Comment at: clang/include/clang/AST/ASTConcept.h:222
- void print(llvm::raw_ostream &OS, PrintingPolicy Policy) const;
+ ConceptLoc *getConceptLoc() { return Loc; }
+ ConceptLoc *getConceptLoc() const { return Loc; }
----------------
FWIW clang doesn't seem to usually bother with const-correctness, so it's hardly even clear what it means.
I'd stick with ConceptLoc *getConceptLoc() const :-)
================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:318
+ /// \returns false if the visitation was terminated early, true otherwise.
+ bool TraverseConceptLoc(const ConceptLoc *CL);
+
----------------
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?
================
Comment at: clang/include/clang/AST/TypeLoc.h:2111
+ ConceptLoc *CL = nullptr;
// Followed by a TemplateArgumentLocInfo[]
----------------
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`.
================
Comment at: clang/include/clang/AST/TypeLoc.h:2112
+ ConceptLoc *CL = nullptr;
// Followed by a TemplateArgumentLocInfo[]
};
----------------
It looks like this is not necessary now, as we can reuse the TemplateArgument in `ConceptLoc`, maybe we can even remove it in the future
================
Comment at: clang/include/clang/AST/TypeLoc.h:2140
+ const NestedNameSpecifierLoc getNestedNameSpecifierLoc() const {
+ if (getConceptLoc())
+ return getConceptLoc()->getNestedNameSpecifierLoc();
----------------
nit: `if (const auto* Concept = getConcetLoc())`, the same for the other places.
any reason why we need this null sanity check?
================
Comment at: clang/include/clang/AST/TypeLoc.h:2198
+ return getConceptLoc()->getTemplateArgsAsWritten()->getTemplateArgs()[i];
+ // FIXME: Check why we still need this fallback in case that there is no
+ // ConceptLoc.
----------------
which kind of tests were broken if we remove this?
================
Comment at: clang/include/clang/AST/TypeLoc.h:2158
+ return getConceptLoc()->getTemplateKWLoc();
+ return getDefaultLoc();
}
----------------
I cannot replace this by `SourceLocation()` without failing tests, although this function should only be called if there are constraints.
================
Comment at: clang/include/clang/AST/TypeLoc.h:2158
+ return getConceptLoc()->getTemplateKWLoc();
+ return getDefaultLoc();
}
----------------
massberg wrote:
> I cannot replace this by `SourceLocation()` without failing tests, although this function should only be called if there are constraints.
is this comment still applied (you marked it done)? It seems reasonable to return a `SourceLocation()`, which tests were failed if we did this?
================
Comment at: clang/include/clang/AST/TypeLoc.h:2158
+ return getConceptLoc()->getTemplateKWLoc();
+ return getDefaultLoc();
}
----------------
hokein wrote:
> massberg wrote:
> > I cannot replace this by `SourceLocation()` without failing tests, although this function should only be called if there are constraints.
> is this comment still applied (you marked it done)? It seems reasonable to return a `SourceLocation()`, which tests were failed if we did this?
My original comment had become obsolete.
================
Comment at: clang/include/clang/AST/TypeLoc.h:2187
SourceLocation getLAngleLoc() const {
return this->getLocalData()->LAngleLoc;
----------------
I assume that `getLAngleLoc` and `getRAngleLoc` should be moved to `ConceptLoc`, even if it currently only used by `AutoTypeLoc`?
================
Comment at: clang/include/clang/AST/TypeLoc.h:2187
SourceLocation getLAngleLoc() const {
return this->getLocalData()->LAngleLoc;
----------------
massberg wrote:
> I assume that `getLAngleLoc` and `getRAngleLoc` should be moved to `ConceptLoc`, even if it currently only used by `AutoTypeLoc`?
This might be accessible from `getConceptLoc()->getTemplateArgsAsWritten().
================
Comment at: clang/include/clang/AST/TypeLoc.h:2212-2213
+ // ConceptLoc.
return TemplateArgumentLoc(getTypePtr()->getTypeConstraintArguments()[i],
- getArgLocInfo(i));
+ getArgInfos()[i]);
}
----------------
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.
================
Comment at: clang/lib/AST/ASTImporter.cpp:5640
Error Err = Error::success();
auto ToNNS = importChecked(Err, TC->getNestedNameSpecifierLoc());
auto ToName = importChecked(Err, TC->getConceptNameInfo().getName());
----------------
better to instead import via the TC->getConceptLoc(), and have a function to wrap up the import of the individual properties
================
Comment at: clang/lib/AST/ASTImporter.cpp:5657
+ Importer.getToContext(), ToNNS,
+ ToNameLoc, // What is the right Loc here?
+ DeclarationNameInfo(ToName, ToNameLoc), ToFoundDecl, ToNamedConcept,
----------------
the templateKWLoc should be the import of the original constraint's conceptloc's templatekwloc
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:1274
+ auto *CL = ConceptLoc::Create(Context, /*NNS=*/NS,
+ /*TemplateKWLoc=*/S,
+ /*ConceptNameInfo=*/NameInfo,
----------------
>From reading the code that currently initializes ConceptReference for a TypeConstraint, this is always SourceLocation{}.
Either this is not possible at the language level, or it's just not something the AST currently tracks.
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:2632
+ Reader.getContext(), NNS,
+ SourceLocation{}, // What is the correct value here?
+ DN, /*FoundDecl=*/nullptr, NamedConcept, ArgsAsWritten);
----------------
again, this is correct, at least as correct as current code.
type constraints never have the `template` keyword, or it's never tracked
This patch adds a concept AST node (`ConceptLoc`) and uses it at the corresponding places.
There are three objects that might have constraints via concepts:
`TypeConstraint`, `ConceptSpecializationExpr` and `AutoTypeLoc`.
The first two inherit from `ConceptReference` while the latter has
the information about a possible constraint directly stored in `AutoTypeLocInfo`. It would be nice if the concept information would be stored the same way in all three cases.
Moreover the current structure makes it difficult to deal with these concepts. For example in Clangd accessing the locations of constraints of a `AutoTypeLoc` can only be done with quite ugly hacks.
So we think that it makes sense to create a new AST node for such concepts.
In details we propose the following:
- Rename `ConceptReference` to `ConceptLoc` (or something else what is approriate)
and make it the new AST node.
- `TypeConstraint` and `ConceptSpecializationExpr` do not longer inherit from `ConceptReference` but store a pointer to a `ConceptLoc`.
- `AutoTypeLoc` stores a pointer to `ConceptLoc` instead of storing the concept info in `AutoTypeLocInfo`.
This patch implements a first version of this idea which compiles and where the existing tests pass.
To make this patch as small as possible we keep the existing member functions to access concept data. Later these can be replaced by directly calling the corresponding functions of the `ConceptLoc`s.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D155858
Files:
clang/include/clang/AST/ASTConcept.h
clang/include/clang/AST/DeclTemplate.h
clang/include/clang/AST/ExprConcepts.h
clang/include/clang/AST/RecursiveASTVisitor.h
clang/include/clang/AST/TypeLoc.h
clang/include/clang/Serialization/ASTRecordReader.h
clang/include/clang/Serialization/ASTRecordWriter.h
clang/lib/AST/ASTContext.cpp
clang/lib/AST/ASTImporter.cpp
clang/lib/AST/DeclTemplate.cpp
clang/lib/AST/ExprConcepts.cpp
clang/lib/AST/TypeLoc.cpp
clang/lib/Sema/SemaTemplate.cpp
clang/lib/Sema/SemaTemplateInstantiate.cpp
clang/lib/Sema/SemaType.cpp
clang/lib/Sema/TreeTransform.h
clang/lib/Serialization/ASTReader.cpp
clang/lib/Serialization/ASTReaderDecl.cpp
clang/lib/Serialization/ASTReaderStmt.cpp
clang/lib/Serialization/ASTWriter.cpp
clang/lib/Serialization/ASTWriterDecl.cpp
clang/lib/Serialization/ASTWriterStmt.cpp
clang/unittests/Tooling/RecursiveASTVisitorTests/Concept.cpp
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D155858.546339.patch
Type: text/x-patch
Size: 46615 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230802/dff37537/attachment-0001.bin>
More information about the cfe-commits
mailing list