[clang-tools-extra] [clang] [clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land) (PR #71417)
steakhal wrote: Thank you all participating, and especially for @Endilll committing the fix as cc3fd1974696a792ba70ba670ed761937cd0735c. Consider my [issue](https://github.com/llvm/llvm-project/pull/71417#issuecomment-1897925793) resolved. :) https://github.com/llvm/llvm-project/pull/71417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang] [clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land) (PR #71417)
AaronBallman wrote: > > I had an offline discussion with @Endilll during my morning office hours > > today, and our current plan is: > > > > 1. Remove `Implicit` from the enumeration, rename `Call` and `List` to > > `ParenList` and `BraceList`, respectively > > 2. Add a new bit to the bit-field for `CXXNewExpr` to track "has an > > initializer" instead of encoding it as in-band information in the > > initialization style. > > 3. Use that new bit internally in `CXXNewExpr`, update serialization and > > whatnot accordingly. > > > > This should bring us back to the enumeration mapping directly to syntax but > > removes the strange in-band nature of how the original enumeration was > > being used. > > Thank you! I really like how this direction, that both keeps the benefit of > simplified implementation and makes the enumeration values cleaner. Of course > this resolves, all concerns that we have with this change. Thank you for raising the concern, I think we ended up with a cleaner solution in the end! https://github.com/llvm/llvm-project/pull/71417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang] [clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land) (PR #71417)
AaronBallman wrote: I had an offline discussion with @Endilll during my morning office hours today, and our current plan is: 1) Remove `Implicit` from the enumeration, rename `Call` and `List` to `ParenList` and `BraceList`, respectively 2) Add a new bit to the bit-field for `CXXNewExpr` to track "has an initializer" instead of encoding it as in-band information in the initialization style. 3) Use that new bit internally in `CXXNewExpr`, update serialization and whatnot accordingly. This should bring us back to the enumeration mapping directly to syntax but removes the strange in-band nature of how the original enumeration was being used. https://github.com/llvm/llvm-project/pull/71417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang] [clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land) (PR #71417)
tomasz-kaminski-sonarsource wrote: And what will the desired use of having the distinction `Implicit` vs `NoInit` in its current form? The current differentiates between the allocation object of class `Trivial` and `int[2]` where there, actually there is none in the emitted code. And proper analysis of the initializer is needed anyway. I would be very supportive of updating the names, so we replace `Call` with `ParenList` and `List` with `BraceList` as that will reduce the confusion. This is regardless of the effort that is required on downstream. But this PR added a new enum that is equivalent to `getInit() != nullptr`, that I see no use of. If that would be a separate method, that would check if the new performs non-trivial initialization of the object in the allocated memory, it would be also very welcomed and usefull. https://github.com/llvm/llvm-project/pull/71417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang] [clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land) (PR #71417)
cor3ntin wrote: Would it be helpful for you if we: - Introduce `getInitializationStyleAsWritten` and `hasInitializerAsWritten` methods (that map `Implicit` to `NoInit`) - Take the opportunity to rename `Call/List` to `Paren/BraceInitialization` ? https://github.com/llvm/llvm-project/pull/71417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang] [clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land) (PR #71417)
steakhal wrote:
The commit _"[clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land)
(#71417)"_
https://github.com/llvm/llvm-project/commit/e929f0694aeb5f8cdbd2369db6189d28bb6fbcf3
appears to be a functional change, as it has a side effect on the following
test code:
```diff
diff --git a/clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
b/clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
index 8f0dd5602307..59a1a673dd99 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -2738,6 +2738,35 @@ TEST(MatchFinderAPI, MatchesDynamic) {
EXPECT_EQ(MethodNode, GlobalMethodNode);
}
+TEST(MatchFinderAPI, InvalidParenOrBraceRange) {
+ StringRef SourceCode = R"cpp(
+struct Base {
+ Base(int x, int y, int z);
+};
+class Derived : public Base {
+public:
+ Derived();
+ explicit Derived(int x);
+};
+void top() {
+ const auto *p = new Derived;
+}
+ )cpp";
+
+ // master: getInitializationStyle() == CXXNewExpr::NoInit
+
+ auto Matcher = cxxNewExpr(has(cxxConstructExpr())).bind("target");
+ auto AstUnit = tooling::buildASTFromCode(SourceCode);
+ auto Matches = matchDynamic(Matcher, AstUnit->getASTContext());
+ const auto = AstUnit->getSourceManager();
+ ASSERT_EQ(Matches.size(), 1u);
+ ASSERT_EQ(Matches[0].getMap().size(), 1u);
+ const auto *NewExpr = Matches[0].getNodeAs("target");
+ ASSERT_TRUE(NewExpr != nullptr);
+ int Actual = (int)NewExpr->getInitializationStyle();
+ ASSERT_EQ(0, Actual);
+}
+
static std::vector allTestClangConfigs() {
std::vector all_configs;
for (TestLanguage lang : {Lang_C89, Lang_C99, Lang_CXX03, Lang_CXX11,
```
The assertion `ASSERT_EQ(0, Actual)` would pass on the commit prior to
e929f0694aeb5f8cdbd2369db6189d28bb6fbcf3, and break with it, and report that
the `Actual` is `1`.
Reproducer instructions after applying the patch:
```bash
ninja -C build/rel ASTMatchersTests
./build/rel/tools/clang/unittests/ASTMatchers/ASTMatchersTests
--gtest_filter=MatchFinderAPI.InvalidParenOrBraceRange
```
I'd qualify this as a regression, by looking at that the commit was supposed to
be an NFC.
Could you please confirm @Endilll?
https://github.com/llvm/llvm-project/pull/71417
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang] [clang][NFC] Refactor `CXXNewExpr::InitializationStyle` (re-land) (PR #71417)
https://github.com/Endilll closed https://github.com/llvm/llvm-project/pull/71417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
