After some discussion, it was decided to allow not only identifiers as
context selectors, but also string literals.
Details
- Reviewers
jdoerfert aaron.ballman
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Build result: pass - 60439 tests passed, 0 failed and 726 were skipped.
Log files: console-log.txt, CMakeCache.txt
Thx for this patch. Only two minor comments.
clang/include/clang/Basic/DiagnosticParseKinds.td | ||
---|---|---|
1237 | I would have expected this error to be still accurate, maybe with the addition ", or quoted versions thereof". | |
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp | ||
424 | When would ER not be usable here? Is there a valid use case or should we assert it is? |
clang/include/clang/Basic/DiagnosticParseKinds.td | ||
---|---|---|
1237 | Currently, we could emit it only in codegen phase to avoid double converting from expression to string. Will emit it there. | |
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp | ||
424 | If the instantiation is failed, the error message is emitted and ER is set to ExprError, which is not usable. |
Build result: pass - 60475 tests passed, 0 failed and 726 were skipped.
Log files: console-log.txt, CMakeCache.txt
clang/include/clang/Basic/DiagnosticParseKinds.td | ||
---|---|---|
1237 | The main problem here is the conversion and expression evaluation. We convert the data from the expression to strings at the codegen phase. I don't want to do the same thing for the second time in Sema just for diagnostic. |
clang/include/clang/Basic/DiagnosticParseKinds.td | ||
---|---|---|
1237 | We have to convert it during sema already, actually during parsing. I'm working on declare variant begin/end support right now (part of TR8) which needs the information during parsing. |
clang/include/clang/Basic/DiagnosticParseKinds.td | ||
---|---|---|
1237 | Hmmm, it is where we need to parse the code only if the context matches the trait? Ok, will rework to convert it in sema too. Seems to me, we'll need to match the context in both, codegen and sema. Will try to move the matcher somewhere to avoid duplication in sema and codege. |
clang/include/clang/Basic/DiagnosticParseKinds.td | ||
---|---|---|
1237 | I put my patch up later tonight or tomorrow. It moves stuff around, we need to coordinate somehow. |
clang/include/clang/Basic/DiagnosticParseKinds.td | ||
---|---|---|
1237 | I still won't work on it until Monday. So, if you have a good solution for the matcher, go ahead. |
Reworked attribute translationto try to reuse the context matching and scoring functionality with Sema.
Build result: pass - 60659 tests passed, 0 failed and 726 were skipped.
Log files: console-log.txt, CMakeCache.txt
Build result: pass - 60659 tests passed, 0 failed and 726 were skipped.
Log files: console-log.txt, CMakeCache.txt
Unit tests: pass. 60709 tests passed, 0 failed and 726 were skipped.
clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or apply this patch.
Build artifacts: console-log.txt, CMakeCache.txt, clang-format.patch, test-results.xml, diff.json
Unit tests: pass. 60709 tests passed, 0 failed and 726 were skipped.
clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or apply this patch.
Build artifacts: console-log.txt, CMakeCache.txt, clang-format.patch, test-results.xml, diff.json
I haven't forgotten about this! With the other two changes to the declare variant going to happen now,as well, I figured we should look at this again from a high-level.
My plan now is:
- Move all of the declare variant context stuff out of clang into the libFrontendOpenMP. This has various benefits, including reuse capabilities and the problem of where to put it (which was discussed briefly in my other declare variant patches), is gone.
- Set it up to accept *constant* strings and non-string alike from the beginning. I'll also make it completely OMPKinds.def based with all the trait sets and selectors at least parsed properly. Rules what can be nested together, what can have a score, etc. are all reusable and mostly generated.
- Introduce the declare variant begin/end stuff to get math support before the 10 fork.
- Make all declare variant decisions in SemaOverload.
Part 1) and 2) are ready for review (D71830). I took some code from here to handle string literals the same as identifiers and I sprinkled a few '"' into the match clauses to test it.
(In contrast to this patch, with D71830 we will actually remove the quotes when we pretty print the AST instead of adding them.)
I would have expected this error to be still accurate, maybe with the addition ", or quoted versions thereof".