[PATCH] D49223: [AST] Check described template at structural equivalence check.
a.sidorin accepted this revision.
a.sidorin added a comment.
Hi Balazs,
I have only two nits. Otherwise, the patch is OK and can be committed without
additional approval after the comments are fixed. Thank you!
Comment at: lib/AST/ASTStructuralEquivalence.cpp:1500
+bool StructuralEquivalenceContext::CommonCheckAtFinish(Decl *D1, Decl *D2) {
+ // Check for equivalent described template.
CheckCommonEquivalence/CheckKindSpecificEquivalence?
Comment at: lib/AST/ASTStructuralEquivalence.cpp:1643
bool Equivalent = true;
`Equivalent = CommonCheckAtFinish(D1, D2) && SpecialCheckAtFinish(D1, D2))`?
Repository:
rC Clang
https://reviews.llvm.org/D49223
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50428: [ASTImporter] Add support for importing imaginary literals
a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. LGTM! Just a stylish nit. Comment at: lib/AST/ASTImporter.cpp:5617 + + return new (Importer.getToContext()) + ImaginaryLiteral(SubE, T); The line split is not needed here. Repository: rC Clang https://reviews.llvm.org/D50428 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50444: [ASTImporter] Fix structural inequivalency of forward EnumDecl
a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Yes, this seems to be correct. Thanks! Comment at: lib/AST/ASTStructuralEquivalence.cpp:1182 + + // Compare the definitions of these two eunums. If either or both are + // incomplete (i.e. forward declared), we assume that they are equivalent. enums Repository: rC Clang https://reviews.llvm.org/D50444 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50451: [ASTImporter] Fix import of class templates partial specialization
a_sidorin added a comment. Hi Gabor, While importing methods looks harmless, importing fields can be a breaking change. Do you have any test results on this? Comment at: lib/AST/ASTImporter.cpp:2872 Importer.MapImported(D, FoundField); +// In case of a FieldDecl of a ClassTemplateSpecializationDecl, the +// initializer of a FieldDecl might not had been instantiated in the Honestly speaking, I wonder about this behaviour because it doesn't look similar to instantiation of only methods that are used. Is it a common rule? Comment at: lib/AST/ASTImporter.cpp:4550 + // in the "From" context, but not in the "To" context. + for (auto *FromField : D->fields()) +Importer.Import(FromField); Importing additional fields can change the layout of the specialization. For CSA, this usually results in strange assertions hard to debug. Could you please share the results of testing of this change? This change also doesn't seem to have related tests in this patch. Comment at: lib/AST/ASTImporter.cpp:4551 + for (auto *FromField : D->fields()) +Importer.Import(FromField); + The result of import is unchecked here and below. Is it intentional? Comment at: unittests/AST/ASTImporterTest.cpp:2722 + ASSERT_FALSE(ToFun->hasBody()); + auto *ImportedSpec = Import(FromSpec, Lang_CXX11); + ASSERT_TRUE(ImportedSpec); 3-space indentation. Comment at: unittests/AST/ASTImporterTest.cpp:2763 + ASSERT_FALSE(ToCtor->hasBody()); + auto *ImportedSpec = Import(FromSpec, Lang_CXX11); + ASSERT_TRUE(ImportedSpec); Broken indent. Repository: rC Clang https://reviews.llvm.org/D50451 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46940: [ASTImporter] make sure that ACtx::getParents still works
a_sidorin added a comment. Hello Richard, Thank you for clarification. However, I think that moving ParentMap into libTooling is out of scope for this patch. Are you OK if this change will be committed with adding a TODO or FIXME for this move? https://reviews.llvm.org/D46940 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49798: [ASTImporter] Adding some friend function related unittests.
a_sidorin added a comment. Hi Balazs, The patch looks mostly fine. Comment at: unittests/AST/ASTImporterTest.cpp:2256 +FirstDeclMatcher().match(FromTU, cxxRecordDecl()); +auto lookup_res = Class->noload_lookup(FromName); +ASSERT_EQ(lookup_res.size(), 0u); LLVM naming conventions require it to be `LookupRes`. Comment at: unittests/AST/ASTImporterTest.cpp:2258 +ASSERT_EQ(lookup_res.size(), 0u); +lookup_res = cast(FromTU)->noload_lookup(FromName); +ASSERT_EQ(lookup_res.size(), 1u); getTuDecl() already return `TranslationUnitDecl *`. We should just declare `FromTU` as `auto *`, so no cast is needed here. Comment at: unittests/AST/ASTImporterTest.cpp:2279 + EXPECT_TRUE(To0->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend)); + EXPECT_TRUE(!To0->isInIdentifierNamespace(Decl::IDNS_Ordinary)); +} EXPECT_FALSE can be used instead of negations. Comment at: unittests/AST/ASTImporterTest.cpp:2294 +Lang_CXX, "input0.cc"); + auto From0 = FirstDeclMatcher().match(FromTU, Pattern); + auto From1 = LastDeclMatcher().match(FromTU, Pattern); Names like FromFriendFunc/FromNormalFunc will make the test more readable. Repository: rC Clang https://reviews.llvm.org/D49798 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50516: [ASTImporter] Improved import of friend templates.
a_sidorin accepted this revision. a_sidorin added inline comments. This revision is now accepted and ready to land. Comment at: unittests/AST/ASTImporterTest.cpp:2721 + EXPECT_EQ(ToFriendClass->getDefinition(), ToClass); + ASSERT_EQ(ToFriendClass->getPreviousDecl(), ToClass); + ASSERT_EQ( Should we use EXPECT_EQ for "To" checks? Repository: rC Clang https://reviews.llvm.org/D50516 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50552: [ASTImporter] Added test case for CXXConversionDecl importing
a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Thanks! Repository: rC Clang https://reviews.llvm.org/D50552 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50550: [ASTImporter] Added test case for opaque enums
a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D50550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished
a_sidorin updated this revision to Diff 160477.
a_sidorin edited the summary of this revision.
a_sidorin added a comment.
All declarations are reordered now, not only fields. Also some review comments
were addressed.
Repository:
rC Clang
https://reviews.llvm.org/D44100
Files:
lib/AST/ASTImporter.cpp
unittests/AST/ASTImporterTest.cpp
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1428,7 +1428,7 @@
}
TEST_P(ASTImporterTestBase,
- DISABLED_CXXRecordDeclFieldOrderShouldNotDependOnImportOrder) {
+ CXXRecordDeclFieldOrderShouldNotDependOnImportOrder) {
Decl *From, *To;
std::tie(From, To) = getImportedDecl(
// The original recursive algorithm of ASTImporter first imports 'c' then
@@ -2995,5 +2995,16 @@
ImportFunctionTemplateSpecializations,
DefaultTestValuesForRunOptions, );
+TEST_P(ImportDecl, ImportFieldOrder) {
+ MatchVerifier Verifier;
+ testImport("struct declToImport {"
+ " int b = a + 2;"
+ " int a = 5;"
+ "};",
+ Lang_CXX11, "", Lang_CXX11, Verifier,
+ recordDecl(hasFieldOrder({"b", "a"})));
+}
+
+
} // end namespace ast_matchers
} // end namespace clang
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1299,6 +1299,36 @@
if (!Importer.Import(From))
return true;
+ // Reorder declarations in RecordDecls because they may have another
+ // order. Keeping field order is vitable because it determines structure
+ // layout.
+ // FIXME: This is an ugly fix. Unfortunately, I cannot come with better
+ // solution for this issue. We cannot defer expression import here because
+ // type import can depend on them.
+ const auto *FromRD = dyn_cast(FromDC);
+ if (!FromRD)
+return false;
+
+ auto ImportedDC = Importer.Import(cast(FromDC));
+ assert(ImportedDC);
+ auto *ToRD = cast(*ImportedDC);
+
+ for (auto *D : FromRD->decls()) {
+Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+assert(ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD));
+ToRD->removeDecl(ToD);
+ }
+
+ assert(ToRD->decls_empty());
+
+ for (auto *D : FromRD->decls()) {
+Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+assert(ToRD == ToD->getDeclContext());
+assert(ToRD == ToD->getLexicalDeclContext());
+assert(!ToRD->containsDecl(ToD));
+ToRD->addDeclInternal(ToD);
+ }
+
return false;
}
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1428,7 +1428,7 @@
}
TEST_P(ASTImporterTestBase,
- DISABLED_CXXRecordDeclFieldOrderShouldNotDependOnImportOrder) {
+ CXXRecordDeclFieldOrderShouldNotDependOnImportOrder) {
Decl *From, *To;
std::tie(From, To) = getImportedDecl(
// The original recursive algorithm of ASTImporter first imports 'c' then
@@ -2995,5 +2995,16 @@
ImportFunctionTemplateSpecializations,
DefaultTestValuesForRunOptions, );
+TEST_P(ImportDecl, ImportFieldOrder) {
+ MatchVerifier Verifier;
+ testImport("struct declToImport {"
+ " int b = a + 2;"
+ " int a = 5;"
+ "};",
+ Lang_CXX11, "", Lang_CXX11, Verifier,
+ recordDecl(hasFieldOrder({"b", "a"})));
+}
+
+
} // end namespace ast_matchers
} // end namespace clang
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1299,6 +1299,36 @@
if (!Importer.Import(From))
return true;
+ // Reorder declarations in RecordDecls because they may have another
+ // order. Keeping field order is vitable because it determines structure
+ // layout.
+ // FIXME: This is an ugly fix. Unfortunately, I cannot come with better
+ // solution for this issue. We cannot defer expression import here because
+ // type import can depend on them.
+ const auto *FromRD = dyn_cast(FromDC);
+ if (!FromRD)
+return false;
+
+ auto ImportedDC = Importer.Import(cast(FromDC));
+ assert(ImportedDC);
+ auto *ToRD = cast(*ImportedDC);
+
+ for (auto *D : FromRD->decls()) {
+Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+assert(ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD));
+ToRD->removeDecl(ToD);
+ }
+
+ assert(ToRD->decls_empty());
+
+ for (auto *D : FromRD->decls()) {
+Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+assert(ToRD == ToD->getDeclContext());
+assert(ToRD == ToD->getLexicalDeclContext());
+assert(!ToRD->containsDecl(ToD));
+ToRD->addDeclInternal(ToD);
+ }
+
return false;
}
__
[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished
a_sidorin marked 2 inline comments as done.
a_sidorin added inline comments.
Comment at: lib/AST/ASTImporter.cpp:1029
+
+ RecordDecl *ToRD = cast(Importer.Import(cast(FromDC)));
+
martong wrote:
> Can't we just import the `FromRD`, why we need that cast at the end?
> `auto *ToRD = cast(Importer.Import(FromRD)));`
We need to cast it to DeclContext anyway, so I don't think that a narrower type
will be worse
Comment at: lib/AST/ASTImporter.cpp:1032
+ for (auto *FromField : FromRD->fields()) {
+Decl *ToField = Importer.GetAlreadyImportedOrNull(FromField);
+assert(ToRD == ToField->getDeclContext() && ToRD->containsDecl(ToField));
martong wrote:
> I think `ToField` can be a nullptr, and if so, then
> `ToField->getDeclContext()` is UB.
> Same could happen at line 1040.
> Perhaps we should have and explicit check about the nullptr value:
> `if (!ToField) continue;`
>
I have added a check for the return result into the import loop. So, after the
import is finished, all nested decls should be non-null.
Repository:
rC Clang
https://reviews.llvm.org/D44100
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50731: [ASTImporter] Add test for ExprWithCleanups
a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Tests are always welcome. Thanks! Repository: rC Clang https://reviews.llvm.org/D50731 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished
a_sidorin marked an inline comment as done.
a_sidorin added inline comments.
Comment at: lib/AST/ASTImporter.cpp:1317
+ for (auto *D : FromRD->decls()) {
+Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+assert(ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD));
martong wrote:
> Is it sure that `ToD` will never be a nullptr?
> I think, `removeDecl` or `addDeclInternal` below may crash if we call it with
> a nullptr.
> Also in the assert, `ToD->getDeclContext()` seems achy if `ToD` is a nullptr.
We have an early return if such import failed before (line 1300).
Repository:
rC Clang
https://reviews.llvm.org/D44100
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50672: [ASTImporter] Change the return result of Decl import to Optional
a_sidorin added a comment. Hello Gabor and Balazs, > With Balazs, we are working on something similar, but with a different, fine > grained error value mechanism. Unfortunately we were not aware of that you > have been working on error handling, and we didn't say that we are working on > error handling recently, I am terribly sorry about this. That's a bit disappointing because I was thinking that the status of error handling strategy discussion and implementation is reflected in the mailing list. But well, let's think what we can do about this. > From the CSA perspective, we realized that there may be several different > error cases which has to be handled differently in `CrossTranslationUnit.cpp`. > For example, there are unsupported constructs which we do not support to > import (like a struct definition as a parameter of a function). > Another example is when there is a name conflict between the decl in the > "From" context and the decl in the "To" context, this usually means an ODR > error. > We have to handle these errors in a different way after we imported one > function during CTU analysis. > The fact that there may be more than one kind of errors yields for the use > of the designated LLVM types: `Error` and `Expected`. A simple `Optional` > is probably not generic enough. Yes, I was thinking about it too. The reason why I chose `Optional` was that `ASTImporter` clients don't use the error kind. If you have any plans on changing this, `Expected` is a preferred approach. > I find the `importNonNull` and generally the new family of `import` functions > useful, but I am not sure how they could cooperate with `Expected`. > Especially, I have some concerns with output parameters. > If we have an Expected as a result type, then there is no way to acquire > the T if there was an error. However, when we have output parameters, then > even if there was an error some output params could have been set ... and > those can be reused even after the return. If error handling is not proper on > the callee site then we may continue with stale values, which is not possible > if we use Expected as a return value. If I understand you correctly, `importNonNull` and `importNullable` should work with `Expected` pretty well. Changing `import*Into` return result to `Error` can partially help in avoiding usage of an unchecked result. These functions already guarantee that their parameters are changed only if the internal import was successful. > Do you think we can hold back this patch for a few days until Balazs prepares > the `Expected` based version? Then I think we could compare the patches > and we could merge the best from the two of them. Sure. Repository: rC Clang https://reviews.llvm.org/D50672 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49798: [ASTImporter] Adding some friend function related unittests.
a_sidorin added a comment. Hi Balasz, Thank you for the fixes. Comment at: unittests/AST/ASTImporterTest.cpp:2258 +ASSERT_EQ(lookup_res.size(), 0u); +lookup_res = cast(FromTU)->noload_lookup(FromName); +ASSERT_EQ(lookup_res.size(), 1u); a_sidorin wrote: > getTuDecl() already return `TranslationUnitDecl *`. We should just declare > `FromTU` as `auto *`, so no cast is needed here. Marked as Done, but not actually done. Comment at: unittests/AST/ASTImporterTest.cpp:2294 +Lang_CXX, "input0.cc"); + auto From0 = FirstDeclMatcher().match(FromTU, Pattern); + auto From1 = LastDeclMatcher().match(FromTU, Pattern); a_sidorin wrote: > Names like FromFriendFunc/FromNormalFunc will make the test more readable. Gentle ping on this. Repository: rC Clang https://reviews.llvm.org/D49798 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50732: [ASTImporter] Add test for CXXDefaultInitExpr
a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Thanks! > As a side note: It seems this test case actually reveals that we don't import > the body of Foo's destructor? This is strange. If you manage to find the reason, please notify us! Repository: rC Clang https://reviews.llvm.org/D50732 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50737: [ASTImporter] Add test for CXXNoexceptExpr
a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Thanks! Repository: rC Clang https://reviews.llvm.org/D50737 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50932: [ASTImporter] Add test for C++ casts and fix broken const_cast importing.
a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Thank you! Comment at: tools/clang-import-test/clang-import-test.cpp:197 Inv->getLangOpts()->DollarIdents = true; + Inv->getLangOpts()->RTTI = true; Inv->getCodeGenOpts().setDebugInfo(codegenoptions::FullDebugInfo); Is this option set to check `dynamic_cast`? Repository: rC Clang https://reviews.llvm.org/D50932 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50451: [ASTImporter] Fix import of class templates partial specialization
a_sidorin added inline comments. Comment at: lib/AST/ASTImporter.cpp:4550 + // in the "From" context, but not in the "To" context. + for (auto *FromField : D->fields()) +Importer.Import(FromField); martong wrote: > martong wrote: > > a_sidorin wrote: > > > Importing additional fields can change the layout of the specialization. > > > For CSA, this usually results in strange assertions hard to debug. Could > > > you please share the results of testing of this change? > > > This change also doesn't seem to have related tests in this patch. > > TLDR; We will not create additional fields. > > > > By the time when we import the field, we already know that the existing > > specialization is structurally equivalent with the new one. > > Since a ClassTemplateSpecializationDecl is the descendant of RecordDecl, > > the structural equivalence check ensures that they have the exact same > > fields. > > When we import the field of the new spec and if there is an existing > > FieldDecl in the "To" context, then no new FieldDecl will be created (this > > is handled in `VisitFieldDecl` by first doing a lookup of existing field > > with the same name and type). > > This patch extends `VisitFieldDecl` in a way that we add new initializer > > expressions to the existing FieldDecl, if it didn't have and in the "From" > > context it has. > > > > For the record, I added a new test to make sure that a new FieldDecl will > > not be created during the merge. > This is the new test: > `ODRViolationOfClassTemplateSpecializationsShouldBeReported`. It checks that > it is not possible to add new fields to a specialization, rather an ODR > violation is diagnosed. Thank you for the explanation. However, I find the comment very misleading. It tells: ``` // Check and merge those fields which have been instantiated // in the "From" context, but not in the "To" context. ``` Would it be correct to change it to "Import field initializers that are still not instantiated", or do I still misunderstand something? Repository: rC Clang https://reviews.llvm.org/D50451 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50978: [ASTImporter] Add test for C++'s try/catch statements.
a_sidorin accepted this revision. a_sidorin added inline comments. This revision is now accepted and ready to land. Comment at: tools/clang-import-test/clang-import-test.cpp:199 Inv->getLangOpts()->RTTI = true; + Inv->getLangOpts()->Exceptions = true; + Inv->getLangOpts()->CXXExceptions = true; Could you please add a newline after RTTI? Now, it looks like the comment is for all lines below. Repository: rC Clang https://reviews.llvm.org/D50978 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50662: Add dump() method for SourceRange
a_sidorin requested changes to this revision. a_sidorin added a comment. This revision now requires changes to proceed. Hello Stephen, These methods will be really useful. Comment at: lib/Basic/SourceLocation.cpp:90 + B.print(OS, SM); + OS << ",\n "; + E.print(OS, SM); I think we should print both locations on the same line, without adding a newline after the start SLoc. Comment at: lib/Basic/SourceLocation.cpp:92 + E.print(OS, SM); + OS << "]\n"; +} print() shouldn't add a newline, it is up to dump() methods. Repository: rC Clang https://reviews.llvm.org/D50662 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50451: [ASTImporter] Fix import of class templates partial specialization
a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Thank you! Repository: rC Clang https://reviews.llvm.org/D50451 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast
a.sidorin added subscribers: NoQ, a.sidorin.
a.sidorin added a comment.
Accidentally noticed about this review via cfe-commits. @NoQ, the change in the
ExprEngine looks a bit dangerous to me. Could you please check?
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:419
+ case CK_LValueBitCast:
+ case CK_FixedPointCast: {
state =
Should we consider this construction as unsupported rather than supported as a
normal cast?
Repository:
rC Clang
https://reviews.llvm.org/D50616
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51115: [ASTImporter] Actually test ArrayInitLoopExpr in the array-init-loop-expr test.
a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Wow, I totally overlooked this. Thank you! Repository: rC Clang https://reviews.llvm.org/D51115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51056: [ASTImporter] Add test for SwitchStmt
a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Thank you for working on this! Repository: rC Clang https://reviews.llvm.org/D51056 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46398: [ASTImporterTest] Fix potential use-after-free
a.sidorin updated this revision to Diff 146587.
a.sidorin added a comment.
After a number of attempts of Twine'ifying all Code samples, I decided to
restore the initial state of this code.
Repository:
rC Clang
https://reviews.llvm.org/D46398
Files:
unittests/AST/ASTImporterTest.cpp
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -207,8 +207,8 @@
struct TU {
// Buffer for the context, must live in the test scope.
-StringRef Code;
-StringRef FileName;
+std::string Code;
+std::string FileName;
std::unique_ptr Unit;
TranslationUnitDecl *TUDecl = nullptr;
TU(StringRef Code, StringRef FileName, ArgVector Args)
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -207,8 +207,8 @@
struct TU {
// Buffer for the context, must live in the test scope.
-StringRef Code;
-StringRef FileName;
+std::string Code;
+std::string FileName;
std::unique_ptr Unit;
TranslationUnitDecl *TUDecl = nullptr;
TU(StringRef Code, StringRef FileName, ArgVector Args)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46398: [ASTImporterTest] Fix potential use-after-free
a.sidorin updated this revision to Diff 146589.
a.sidorin added a comment.
Add forgotten context.
Repository:
rC Clang
https://reviews.llvm.org/D46398
Files:
unittests/AST/ASTImporterTest.cpp
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -207,8 +207,8 @@
struct TU {
// Buffer for the context, must live in the test scope.
-StringRef Code;
-StringRef FileName;
+std::string Code;
+std::string FileName;
std::unique_ptr Unit;
TranslationUnitDecl *TUDecl = nullptr;
TU(StringRef Code, StringRef FileName, ArgVector Args)
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -207,8 +207,8 @@
struct TU {
// Buffer for the context, must live in the test scope.
-StringRef Code;
-StringRef FileName;
+std::string Code;
+std::string FileName;
std::unique_ptr Unit;
TranslationUnitDecl *TUDecl = nullptr;
TU(StringRef Code, StringRef FileName, ArgVector Args)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45416: [AST, analyzer] Transform rvalue cast outputs to lvalues (fheinous-gnu-extensions)
a.sidorin added a comment. Gentle ping. Repository: rC Clang https://reviews.llvm.org/D45416 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46353: [ASTImporter] Extend lookup logic in class templates
a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D46353#1086456, @martong wrote: > > should we add this new declaration to the redeclaration chain like we do it > > for RecordDecls? > > I think, on a long term we should. Otherwise we could loose e.g. C++11 > attributes which are attached to the forward declaration only. > However, I'd do that as a separate commit, because that would require some > independent changes and tests, also other decl kinds like > ClassTemplateSepcializationDecl may be affected as well by that. I'm fine with this. But a FIXME would be very appreciated. Repository: rC Clang https://reviews.llvm.org/D46353 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46398: [ASTImporterTest] Fix potential use-after-free
a.sidorin closed this revision. a.sidorin added a comment. Closed with https://reviews.llvm.org/rC332256. Repository: rC Clang https://reviews.llvm.org/D46398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46835: [ASTImporter] Do not try to remove invisible Decls from DeclContext
a.sidorin added a comment.
Hi Gabor,
I don't feel I'm a right person to review AST-related part so I'm adding other
reviewers.
What I'm worrying about is that there is no test to check if our changes in
removeDecl are correct. Maybe https://reviews.llvm.org/D44100 is a right patch
for this but we should land it first or set dependencies properly.
Regarding ASTImporter, you can find my comments inline.
Comment at: lib/AST/DeclBase.cpp:1386
+// Do not try to remove the declaration if that is invisible to qualified
+// lookup. E.g. template sepcializations are skipped.
+if (shouldBeHidden(ND)) return;
specializations
Comment at: unittests/AST/ASTImporterTest.cpp:1770
+TEST(ImportExpr, ImportClassTemplatePartialSpecialization) {
+ MatchVerifier Verifier;
These tests seem to be for ImportDecl, not for ImportExpr.
Comment at: unittests/AST/ASTImporterTest.cpp:1803
+
+ testImport(Code, Lang_CXX11, "", Lang_CXX11, Verifier, namespaceDecl());
+}
Check for namespaceDecl() looks too weak because import of NamespaceDecl
succeeds even if import of its nested decls fails. Same below.
Comment at: unittests/AST/ASTImporterTest.cpp:1827
+
+TEST_P(ASTImporterTestBase, DISABLED_ImportOfRecordWithDifferentFriends) {
+ Decl *ToR1;
For this change, we should create a separate patch.
Repository:
rC Clang
https://reviews.llvm.org/D46835
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46353: [ASTImporter] Extend lookup logic in class templates
This revision was automatically updated to reflect the committed changes.
Closed by commit rC332338: [ASTImporter] Extend lookup logic in class templates
(authored by a.sidorin, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D46353?vs=146765&id=146780#toc
Repository:
rC Clang
https://reviews.llvm.org/D46353
Files:
lib/AST/ASTImporter.cpp
unittests/AST/ASTImporterTest.cpp
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -4108,8 +4108,14 @@
if (auto *FoundTemplate = dyn_cast(Found)) {
if (IsStructuralMatch(D, FoundTemplate)) {
// The class templates structurally match; call it the same template.
- // FIXME: We may be filling in a forward declaration here. Handle
- // this case!
+
+ // We found a forward declaration but the class to be imported has a
+ // definition.
+ // FIXME Add this forward declaration to the redeclaration chain.
+ if (D->isThisDeclarationADefinition() &&
+ !FoundTemplate->isThisDeclarationADefinition())
+continue;
+
Importer.Imported(D->getTemplatedDecl(),
FoundTemplate->getTemplatedDecl());
return Importer.Imported(D, FoundTemplate);
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1431,6 +1431,39 @@
MatchVerifier{}.match(To->getTranslationUnitDecl(), Pattern));
}
+TEST_P(ASTImporterTestBase, ImportDefinitionOfClassTemplateAfterFwdDecl) {
+ {
+Decl *FromTU = getTuDecl(
+R"(
+template
+struct B;
+)",
+Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+
+Import(FromD, Lang_CXX);
+ }
+
+ {
+Decl *FromTU = getTuDecl(
+R"(
+template
+struct B {
+ void f();
+};
+)",
+Lang_CXX, "input1.cc");
+FunctionDecl *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+Import(FromD, Lang_CXX);
+auto *FromCTD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+auto *ToCTD = cast(Import(FromCTD, Lang_CXX));
+EXPECT_TRUE(ToCTD->isThisDeclarationADefinition());
+ }
+}
+
INSTANTIATE_TEST_CASE_P(
ParameterizedTests, ASTImporterTestBase,
::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -4108,8 +4108,14 @@
if (auto *FoundTemplate = dyn_cast(Found)) {
if (IsStructuralMatch(D, FoundTemplate)) {
// The class templates structurally match; call it the same template.
- // FIXME: We may be filling in a forward declaration here. Handle
- // this case!
+
+ // We found a forward declaration but the class to be imported has a
+ // definition.
+ // FIXME Add this forward declaration to the redeclaration chain.
+ if (D->isThisDeclarationADefinition() &&
+ !FoundTemplate->isThisDeclarationADefinition())
+continue;
+
Importer.Imported(D->getTemplatedDecl(),
FoundTemplate->getTemplatedDecl());
return Importer.Imported(D, FoundTemplate);
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1431,6 +1431,39 @@
MatchVerifier{}.match(To->getTranslationUnitDecl(), Pattern));
}
+TEST_P(ASTImporterTestBase, ImportDefinitionOfClassTemplateAfterFwdDecl) {
+ {
+Decl *FromTU = getTuDecl(
+R"(
+template
+struct B;
+)",
+Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+
+Import(FromD, Lang_CXX);
+ }
+
+ {
+Decl *FromTU = getTuDecl(
+R"(
+template
+struct B {
+ void f();
+};
+)",
+Lang_CXX, "input1.cc");
+FunctionDecl *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+Import(FromD, Lang_CXX);
+auto *FromCTD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+auto *ToCTD = cast(Import(FromCTD, Lang_CXX));
+EXPECT_TRUE(ToCTD->isThisDeclarationADefinition());
+ }
+}
+
INSTANTIATE_TEST_CASE_P(
ParameterizedTests, ASTImporterTestBase,
::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
___
[PATCH] D46867: [ASTImporter] Add unit tests for structural equivalence
a.sidorin added a comment.
Hello Gabor!
Thank you for this patch! Do you plan to enable this functionality for AST
import checking?
Some comments are inline.
Comment at: unittests/AST/Language.h:1
+//===- unittest/AST/Language.h - AST unit test support ---===//
+//
Header line is too short.
Comment at: unittests/AST/Language.h:8
+//
+//===--===//
+
Could you please add a brief file description?
Comment at: unittests/AST/Language.h:13
+
+#include
+#include
System includes should follow LLVM includes.
Comment at: unittests/AST/Language.h:37
+
+inline ArgVector getBasicRunOptionsForLanguage(Language Lang) {
+ ArgVector BasicArgs;
I think this function is too big for a header. Same below. Could you create a
source file?
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:2
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTImporter.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
Do we need ASTImporter.h?
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:39
+auto getDecl = [](ASTContext &Ctx, const std::string &Name) -> NamedDecl *
{
+ IdentifierInfo *ImportedII = &Ctx.Idents.get(Name);
+ assert(ImportedII && "Declaration with the identifier "
s/Import/Search?
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:47
+
+ // We should find one Decl but one only
+ assert(FoundDecls.size() > 0);
Nit: comments should end with dot. Could you please check?
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:48
+ // We should find one Decl but one only
+ assert(FoundDecls.size() > 0);
+ assert(FoundDecls.size() < 2);
Can we `assert(FoundDecls.size() == 1)`?
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:54
+
+NamedDecl *d0 = getDecl(Ctx0, Identifier);
+NamedDecl *d1 = getDecl(Ctx1, Identifier);
`D0`, `D1` (capital). Same below.
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:73
+ auto t = makeNamedDecls("int foo;", "int foo;", Lang_CXX);
+ EXPECT_TRUE(testStructuralMatch(get<0>(t), get<1>(t)));
+}
Could we just overload testStructuralMatch to accept tuple or pair?
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:119
+ *cast(get<1>(t))->spec_begin();
+ ASSERT_TRUE(Spec0 != nullptr);
+ ASSERT_TRUE(Spec1 != nullptr);
Should we assert for `spec_begin() != spec_end()` instead or within these
assertions?
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:163
+TEST_F(StructuralEquivalenceTest, DISABLED_WrongOrderInNamespace) {
+ auto Code0 =
+ R"(
There is no Code1, so I think we can call it just "Code". Same below.
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:174
+ )";
+ auto t = makeNamedDecls( Code0, Code0, Lang_CXX);
+
1. It's better to use more meaningful names than `t`. DeclTuple?
2. The space after `(` is redundant.
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:193
+ auto Code0 = "class X { int a; int b; };";
+ auto t = makeNamedDecls( Code0, Code0, Lang_CXX, "X");
+
Redundant space. Could you clang-format?
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:195
+
+ ASSERT_TRUE(get<0>(t) != nullptr);
+ ASSERT_TRUE(get<1>(t) != nullptr);
These assertions are always true because there is a strong C assertion in the
callee.
Repository:
rC Clang
https://reviews.llvm.org/D46867
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished
a.sidorin added a comment. Ping. Repository: rC Clang https://reviews.llvm.org/D44100 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46940: [ASTImporter] make sure that ACtx::getParents still works
a.sidorin added a comment. Hello Rafael, The patch is awesome. The only thing I'm uncomfortable with is that we call invalidation on every stmt import - not only during the top-level one. Fixing this requires separating entry point from internal imports and is out of scope of this patch, but adding some FIXMEs is very appreciated. Comment at: lib/AST/ASTImporter.cpp:6755 + auto ToE = cast_or_null(Import(cast(FromE))); + if (ToE) +ToContext.invalidateParents(); We already do the invalidation in Import(Stmt), so it looks redundant here. Repository: rC Clang https://reviews.llvm.org/D46940 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
a.sidorin added a comment. This is a nice extension of https://reviews.llvm.org/D16063. Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1344 // This is a signed value, since it's used in arithmetic with signed indices. - return svalBuilder.makeIntVal(RegionSize / EleSize, false); + return svalBuilder.makeIntVal(RegionSize / EleSize, Ctx.getSignedSizeType()); } I think we should initialize SValBuilder::ArrayIndexTy with getSignedSizeType() instead of LongLongTy and use `svalBuilder.getArrayIndexType()` here instead. Comment at: test/Analysis/array-index.c:1 +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.security.ArrayBound,alpha.unix.cstring.OutOfBounds -verify -Wno-implicit-function-declaration %s + Can we place these tests into Analysis/index-type.c? Repository: rC Clang https://reviews.llvm.org/D46944 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
a.sidorin added inline comments.
Comment at: test/Analysis/array-index.c:11
+
+void fie() {
+ buf[SIZE-1] = 1;
Could you please give meaningful names to the tests?
Repository:
rC Clang
https://reviews.llvm.org/D46944
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
a.sidorin added inline comments. Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1344 // This is a signed value, since it's used in arithmetic with signed indices. - return svalBuilder.makeIntVal(RegionSize / EleSize, false); + return svalBuilder.makeIntVal(RegionSize / EleSize, Ctx.getSignedSizeType()); } ebevhan wrote: > a.sidorin wrote: > > I think we should initialize SValBuilder::ArrayIndexTy with > > getSignedSizeType() instead of LongLongTy and use > > `svalBuilder.getArrayIndexType()` here instead. > I made the change, but it caused a spurious out of bounds warning in > index-type.c for the 32-bit case. Making the type signed means that anything > above MAX/2 will break, and the test uses arrays of that size. Hm, yes. ssize_t is 32-bit on 32-bit targets but our indices can exceed it. Even if so, `svalBuilder.getArrayIndexType()` should be fine. Repository: rC Clang https://reviews.llvm.org/D46944 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46950: [ASTImporter] Fix duplicate class template definitions problem
a.sidorin added a comment.
Hello Gabor,
The patch is fine, I have found only some style issues.
Comment at: lib/AST/ASTImporter.cpp:4073
+// Returns the definition for a (forwad) declaration of a ClassTemplateDecl, if
+// it has any definition in the redecl chain.
forward
Comment at: lib/AST/ASTImporter.cpp:4121
- // We found a forward declaration but the class to be imported has a
- // definition.
- // FIXME Add this forward declaration to the redeclaration chain.
- if (D->isThisDeclarationADefinition() &&
- !FoundTemplate->isThisDeclarationADefinition())
+// The class to be imported has a definition.
+if (D->isThisDeclarationADefinition()) {
is a definition?
Comment at: lib/AST/ASTImporter.cpp:4124
+ // Lookup will find the fwd decl only if that is more recent than the
+ // definition. So, lets try to get the definition if that is
available
+ // in the redecl chain.
let's; if it is available
Comment at: lib/AST/ASTImporter.cpp:4126
+ // in the redecl chain.
+ ClassTemplateDecl* TemplateWithDef = getDefinition(FoundTemplate);
+ if (!TemplateWithDef)
* is misplaced here.
Comment at: unittests/AST/ASTImporterTest.cpp:1550
+ASTImporterTestBase,
+
ImportDefinitionOfClassTemplateSpecializationIfThereIsAnExistingFwdDeclAndDefinition)
{
+ Decl *ToTU = getToTuDecl(
Although long names make me happy, this one exceeds 80-char limit. We can
abbreviate "Specialization" to "Spec".
Comment at: unittests/AST/DeclMatcher.h:49
+ using UnaryPredicate = std::function;
+ UnaryPredicate predicate;
+ unsigned count = 0;
Predicate, Count (member names should start with capital).
Repository:
rC Clang
https://reviews.llvm.org/D46950
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46958: [ASTImporter] Fix missing implict CXXRecordDecl
a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. So, we fail to add injected name to a CXXRecordDecl that has a described class template? Nice catch! LGTM. Repository: rC Clang https://reviews.llvm.org/D46958 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46940: [ASTImporter] make sure that ACtx::getParents still works
a.sidorin accepted this revision. a.sidorin added a comment. LGTM. Aaron, could you please confirm that AST changes are fine? Comment at: include/clang/AST/ASTContext.h:638 +ReleaseParentMapEntries(); +PointerParents = nullptr; + } I'd prefer to call reset(), but I won't insist on it. https://reviews.llvm.org/D46940 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47069: [ASTImporter] Enable disabled but passing test
a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. Nice! Repository: rC Clang https://reviews.llvm.org/D47069 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47058: [ASTImporter] Fix ClassTemplateSpecialization in wrong DC
a.sidorin accepted this revision.
a.sidorin added a comment.
This revision is now accepted and ready to land.
Hi Gabor! I have a question.
Comment at: lib/AST/ASTImporter.cpp:4305
+// Add to the DC only if it was an explicit specialization/instantiation.
+if (D2->getTemplateSpecializationKind() != TSK_ImplicitInstantiation) {
+ LexicalDC->addDeclInternal(D2);
Can we write `if (D2->isExplicitInstantiationOrSpecialization())` instead? How
we should treat TSK_Undeclared case?
Repository:
rC Clang
https://reviews.llvm.org/D47058
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47058: [ASTImporter] Fix ClassTemplateSpecialization in wrong DC
a.sidorin requested changes to this revision. a.sidorin added a comment. This revision now requires changes to proceed. (Sorry, accepted accidentially). Repository: rC Clang https://reviews.llvm.org/D47058 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47057: [ASTImporter] Fix missing implict CXXRecordDecl in ClassTemplateSpecializationDecl
a.sidorin added inline comments.
Comment at: lib/AST/ASTImporter.cpp:1962
TagDecl *Definition = D->getDefinition();
- if (Definition && Definition != D) {
+ if (!D->isImplicit() && Definition && Definition != D) {
Decl *ImportedDef = Importer.Import(Definition);
We are changing import if RecordDecl. Is it possible to add a test that doesn't
require templates?
I tried and found that the implicit CXXRecordDecl of
ClassTemplateSpecializationDecl is its redeclaration. That's not true for
normal CXXRecordDecls, as I see, so this deserves a comment.
Repository:
rC Clang
https://reviews.llvm.org/D47057
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46950: [ASTImporter] Fix duplicate class template definitions problem
a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. LGTM, thanks! Repository: rC Clang https://reviews.llvm.org/D46950 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47057: [ASTImporter] Fix missing implict CXXRecordDecl in ClassTemplateSpecializationDecl
a.sidorin accepted this revision.
a.sidorin added a comment.
This revision is now accepted and ready to land.
LGTM with a nit.
Comment at: lib/AST/ASTImporter.cpp:1962
TagDecl *Definition = D->getDefinition();
- if (Definition && Definition != D) {
+ if (!D->isImplicit() /*In contrast to a normal CXXRecordDecl, the implicit
+ CXXRecordDecl of ClassTemplateSpecializationDecl is its redeclaration.
Multiline comments are pretty uncommon in LLVM. Could you please replace it
with `//` before commit? I.e.
```
if (Definition && Definition != D &&
//
//
!D->isImplicit())
```
Repository:
rC Clang
https://reviews.llvm.org/D47057
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47058: [ASTImporter] Fix ClassTemplateSpecialization in wrong DC
a.sidorin added a comment. Hi Gabor, Could you add a test for TSK_Undeclared as well? Repository: rC Clang https://reviews.llvm.org/D47058 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
a.sidorin added a comment.
Hi Bevin,
Could you please address these comments?
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:89
SymMgr(context, BasicVals, alloc), MemMgr(context, alloc),
-StateMgr(stateMgr), ArrayIndexTy(context.LongLongTy),
+StateMgr(stateMgr), ArrayIndexTy(context.getSignedSizeType()),
ArrayIndexWidth(context.getTypeSize(ArrayIndexTy)) {}
As you correctly pointed, ssize_t is 32-bit on 32-bit systems. Therefore, it is
too short. So, we can leave this line as-is.
Comment at: test/Analysis/index-type.c:13
char arr[X86_ARRAY_SIZE];
- char *ptr = arr + UINT_MAX/2;
+ char *ptr = arr + UINT_MAX/4;
ptr += 2; // index shouldn't overflow
We don't need to fix the test - it is correct. We have to fix the type instead.
Comment at: test/Analysis/index-type.c:25
+void testOutOfBounds() {
+ // not out of bounds
+ buf[SIZE-1] = 1; // no-warning
The comments should be normal sentences: "Not out of bounds."
https://reviews.llvm.org/D46944
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46867: [ASTImporter] Add unit tests for structural equivalence
a.sidorin accepted this revision.
a.sidorin added a comment.
This revision is now accepted and ready to land.
Hi Gabor,
LGTM, thank you for addressing the comments! Just a minor nit, it's OK to fix
it before committing without a separate review.
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:67
+ bool testStructuralMatch(std::tuple t) {
+using std::get;
+return testStructuralMatch(get<0>(t), get<1>(t));
Not sure we need this using: we can move up the `using` below or just write
std::get twice.
Repository:
rC Clang
https://reviews.llvm.org/D46867
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47058: [ASTImporter] Fix ClassTemplateSpecialization in wrong DC
a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. Ok, I got it, thank you! Repository: rC Clang https://reviews.llvm.org/D47058 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46950: [ASTImporter] Fix duplicate class template definitions problem
a.sidorin added a comment. Hm. Should we test `-fms-compatibility` in addition to `-fdelayed-template-parsing`? Repository: rC Clang https://reviews.llvm.org/D46950 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
a.sidorin added inline comments.
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:89
SymMgr(context, BasicVals, alloc), MemMgr(context, alloc),
-StateMgr(stateMgr), ArrayIndexTy(context.LongLongTy),
+StateMgr(stateMgr), ArrayIndexTy(context.getSignedSizeType()),
ArrayIndexWidth(context.getTypeSize(ArrayIndexTy)) {}
ebevhan wrote:
> a.sidorin wrote:
> > As you correctly pointed, ssize_t is 32-bit on 32-bit systems. Therefore,
> > it is too short. So, we can leave this line as-is.
> But if it's hardcoded to LongLongTy, you have the same problem on 64-bit
> systems.
Some reasons why LongLongTy is used here are listed in D16063. In brief, you
just cannot create an array of size greater than SIZE_MAX/2 on 64-bit
platforms.
https://reviews.llvm.org/D46944
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47445: [ASTImporter] Corrected diagnostic client handling in tests.
a.sidorin accepted this revision. a.sidorin added a comment. Looks good to me, but the approval from AST code owners is required, I think. Repository: rC Clang https://reviews.llvm.org/D47445 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47459: [ASTImporter] Eliminated some unittest warnings.
a.sidorin requested changes to this revision. a.sidorin added a comment. This revision now requires changes to proceed. Hello Balázs! Thank you for addressing this problem, it is really cool. However, it will be much better if we don't suppress warnings but fix them. Could you modify the tests instead? Repository: rC Clang https://reviews.llvm.org/D47459 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47367: [ASTImporter] Add ms compatibility to tests which use the TestBase
a.sidorin added a comment.
> I think, it is overkill to test all possible combinations of the options, we
> should come up with something better here.
I agree with that. I think we need to test just import pairs
{/*From*/no_option, /*To*/no_option}, {option_1, option1}, {option_2,
option_2}, ...{option_n, option_n}.
Another option is to just turn -fno-delayed-template-parsing
-fno-ms-compatibility for ASTImporter tests like it is done in some unit tests,
but I'm not sure it's a correct solution.
Repository:
rC Clang
https://reviews.llvm.org/D47367
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.
a.sidorin added a subscriber: NoQ. a.sidorin added a comment. There are some results for clang and gcc max value for x86 and x64. Source code: const unsigned long long SIZE_MAX = (unsigned long long)(unsigned long)(-1); const unsigned long long size = SIZE_MAX/2; char arr[size+1]; Compiler output: % g++ -c cast-comp.cpp -m32 cast-comp.cpp:6:16: error: size of array ‘arr’ is negative char arr[size+1]; ^ % clang++-6.0 -c cast-comp.cpp -m32 % g++ -c cast-comp.cpp -m32 cast-comp.cpp:6:16: error: size of array ‘arr’ is negative char arr[size+1]; ^ % g++ -c cast-comp.cpp cast-comp.cpp:6:16: error: size of array ‘arr’ is negative char arr[size+1]; ^ % clang++-6.0 -c cast-comp.cpp cast-comp.cpp:6:10: error: array is too large (9223372036854775808 elements) char arr[size+1]; ^~ So, clang accepts indices > SIZE_MAX/2 for x86. For `arr[size]`, only clang-x64 fails with error. I think this means that we need to use LongLongTy as index type, not SizeType. @NoQ, what do you think? https://reviews.llvm.org/D46944 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47313: [ASTImporter] Corrected lookup at import of templated record decl
a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. LGTM too, thank you! Do you need someone to commit this for you? Repository: rC Clang https://reviews.llvm.org/D47313 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D20118: Add support for injected class names and constructor initializers in C++
a.sidorin abandoned this revision. a.sidorin added a comment. This revision seems to be already committed in https://reviews.llvm.org/rC269693, without Differential Revision set. Repository: rL LLVM https://reviews.llvm.org/D20118 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47367: [ASTImporter] Add ms compatibility to tests which use the TestBase
a.sidorin added a comment. I meant that we can use this approach for testImport() too. Repository: rC Clang https://reviews.llvm.org/D47367 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47450: [ASTImporter] Use InjectedClassNameType at import of templated record.
a.sidorin requested changes to this revision.
a.sidorin added a comment.
This revision now requires changes to proceed.
Hello, Balázs,
You can find my comments inline.
Comment at: lib/AST/ASTImporter.cpp:2131
D2CXX->setDescribedClassTemplate(ToDescribed);
+if (!DCXX->isInjectedClassName()) {
+ // In a record describing a template the type should be a
Maybe we should fix it a bit upper (line 2100)?
Comment at: test/ASTMerge/injected-class-name-decl-1/Inputs/inject1.cpp:16
+} // namespace google
+namespace a {
+template class g;
This looks like raw creduce output. Is there a way to simplify this or make
human-readable? Do we really need nested namespaces, unused decls and other
stuff not removed by creduce? I know that creduce is bad at reducing templates
because we often meet similar output internally. But it is usually not a
problem to resolve some redundancy manually to assist creduce. In this case, we
can start by removing `k` and `n`.
We can leave this code as-is only if removing declarations or simplifying
templates affects import order causing the bug to disappear. But even in this
case we have to humanize the test.
Repository:
rC Clang
https://reviews.llvm.org/D47450
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51178: [ASTImporter] Add test for importing anonymous namespaces.
a_sidorin accepted this revision. a_sidorin added inline comments. This revision is now accepted and ready to land. Comment at: test/Import/cxx-anon-namespace/test.cpp:10 +// This is for the nested anonymous namespace. +// CHECK-NEXT: UsingDirectiveDecl +// CHECK-SAME: '' Could you please change a comment a bit and point that implicit using directives are created by Sema for anonymous namespaces? Repository: rC Clang https://reviews.llvm.org/D51178 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51533: [ASTImporter] Merge ExprBits
a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Looks good, thanks! Comment at: unittests/AST/ASTImporterTest.cpp:3241 + auto *ToD = Import(FromD, Lang_CXX11); + ASSERT_TRUE(ToD); + auto *ToInitExpr = cast(ToD)->getAnyInitializer(); EXPECT_TRUE (same below). Repository: rC Clang https://reviews.llvm.org/D51533 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51597: [ASTImporter] Fix import of VarDecl init
a_sidorin added a comment.
Hi Gabor,
The change looks mostly fine but the difference with ASTReader approach
disturbs me a bit.
Comment at: lib/AST/ASTImporter.cpp:1441
+ To->setInit(ToInit);
+ if (From->isInitKnownICE()) {
+EvaluatedStmt *Eval = To->ensureEvaluatedStmt();
I see that this is only a code move but I realized that ASTReader and
ASTImporter handle this case differently. ASTReader code says:
```
if (Val > 1) { // IsInitKnownICE = 1, IsInitNotICE = 2, IsInitICE = 3
EvaluatedStmt *Eval = VD->ensureEvaluatedStmt();
Eval->CheckedICE = true;
Eval->IsICE = Val == 3;
}
```
but ASTimporter sets these bits only if `isInitKnownICE()` is `true`. This
looks strange.
Comment at: lib/AST/ASTImporter.cpp:3190
+// The VarDecl in the "From" context has a definition, but in the
+// "To" context we already has a definition.
+VarDecl *FoundDef = FoundVar->getDefinition();
have (same below)
Comment at: unittests/AST/ASTImporterTest.cpp:3312
+ ASSERT_NE(FromDWithInit, FromDWithDef);
+ ASSERT_EQ(FromDWithDef->getPreviousDecl() ,FromDWithInit);
+
Formatting of comma is broken. Same below.
Repository:
rC Clang
https://reviews.llvm.org/D51597
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49223: [AST] Check described template at structural equivalence check.
a_sidorin added inline comments. Comment at: lib/AST/ASTStructuralEquivalence.cpp:958 + if (D1->isTemplated() != D2->isTemplated()) +return false; I think we can move the changes for both RecordDecl and FunctionDecl into `Finish()` and use `Decl::getDescribedTemplate()`. This will both simplify the patch and give us the support for templated VarDecls and TypeAliasDecls for free. What do you think? Repository: rC Clang https://reviews.llvm.org/D49223 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49792: [ASTmporter] SourceRange-free function parameter checking for declarations
a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. LGTM. Thank you! Repository: rC Clang https://reviews.llvm.org/D49792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49796: [ASTImporter] Load external Decls when getting field index.
a_sidorin added a comment.
Hi Balázs,
The approach is OK but I have some minor comments inline.
Comment at: lib/AST/ASTImporter.cpp:2840
- return Index;
+ assert(false && "Field was not found in its parent context.");
+
`llvm_unreachable`?
Comment at: unittests/AST/ASTImporterTest.cpp:2642
+unsigned ToIndex = 0u;
+for (auto *F : ToLambda->fields()) {
+ if (F == ToField)
I think we can make `getFieldIndex()` a static method of ASTImporter and remove
this loop.
Repository:
rC Clang
https://reviews.llvm.org/D49796
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49223: [AST] Check described template at structural equivalence check.
a_sidorin added inline comments. Comment at: lib/AST/ASTStructuralEquivalence.cpp:958 + if (D1->isTemplated() != D2->isTemplated()) +return false; balazske wrote: > a_sidorin wrote: > > I think we can move the changes for both RecordDecl and FunctionDecl into > > `Finish()` and use `Decl::getDescribedTemplate()`. This will both simplify > > the patch and give us the support for templated VarDecls and TypeAliasDecls > > for free. What do you think? > Yes it can be good to check with `getDescribedClassTemplate` in `Finish`. > (Similarly, there can be more things that are common to check with all `Decl` > or `NamedDecl` objects in `Finish`, specifically the name is checked. Or in > some cases the name does not matter, but in others yes?) I think that name always matters for structure equivalence checking. I cannot remember any case where it was false during development of our PoC. Repository: rC Clang https://reviews.llvm.org/D49223 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42645: New simple Checker for mmap calls
a.sidorin added a comment. Hello David, I have looked into mmap constant definitions in different implementations and found them pretty inconsistent. For example, MMAP_EXEC can be 0x01, 0x04 and I even found 0x00 in some file (https://www.cs.cmu.edu/~dga/crypto/priveth/libethash/mmap.h). Therefore, we should clearly state how do we predict these values. Are you sure that checking `isOSGlibc()` is enough? Also, could you please explain me how the test works? If I understand correctly, for all platforms we manually define the constants in the test. Then, we check if `PROT_WRITE | PROT_EXEC` is set. For OSGlibc, PROT_EXEC is defined as 0x01 in the checker. This means that if isOSGlibc branch is covered, we should not get any warnings for one of test launches because `PROT_WRITE | PROT_EXEC` is 0x03 in the checker and is 0x06 in the test file. Repository: rC Clang https://reviews.llvm.org/D42645 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43012: [ASTImporter] Fix lexical DC for templated decls; support VarTemplatePartialSpecDecl
a.sidorin created this revision.
a.sidorin added reviewers: xazax.hun, szepet, jingham.
Herald added a subscriber: rnkovacs.
Also minor refactoring in related functions was done.
Repository:
rC Clang
https://reviews.llvm.org/D43012
Files:
lib/AST/ASTImporter.cpp
test/ASTMerge/var-cpp/Inputs/var1.cpp
test/ASTMerge/var-cpp/test.cpp
unittests/AST/ASTImporterTest.cpp
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -542,20 +542,32 @@
TEST(ImportType, ImportTypeAliasTemplate) {
MatchVerifier Verifier;
- testImport("template "
- "struct dummy { static const int i = K; };"
- "template using dummy2 = dummy;"
- "int declToImport() { return dummy2<3>::i; }",
- Lang_CXX11, "", Lang_CXX11, Verifier,
- functionDecl(
- hasBody(
- compoundStmt(
- has(
- returnStmt(
- has(
- implicitCastExpr(
- has(
- declRefExpr());
+ testImport(
+ "template "
+ "struct dummy { static const int i = K; };"
+ "template using dummy2 = dummy;"
+ "int declToImport() { return dummy2<3>::i; }",
+ Lang_CXX11, "", Lang_CXX11, Verifier,
+ functionDecl(
+ hasBody(compoundStmt(
+ has(returnStmt(has(implicitCastExpr(has(declRefExpr(,
+ unless(hasAncestor(translationUnitDecl(has(typeAliasDecl()));
+}
+
+const internal::VariadicDynCastAllOfMatcher
+varTemplateSpecializationDecl;
+
+TEST(ImportDecl, ImportVarTemplate) {
+ MatchVerifier Verifier;
+ testImport(
+ "template "
+ "T pi = T(3.1415926535897932385L);"
+ "void declToImport() { pi; }",
+ Lang_CXX11, "", Lang_CXX11, Verifier,
+ functionDecl(
+ hasBody(has(declRefExpr(to(varTemplateSpecializationDecl(),
+ unless(hasAncestor(translationUnitDecl(has(varDecl(
+ hasName("pi"), unless(varTemplateSpecializationDecl();
}
TEST(ImportType, ImportPackExpansion) {
Index: test/ASTMerge/var-cpp/test.cpp
===
--- /dev/null
+++ test/ASTMerge/var-cpp/test.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -emit-pch -std=c++17 -o %t.1.ast %S/Inputs/var1.cpp
+// RUN: %clang_cc1 -std=c++17 -ast-merge %t.1.ast -fsyntax-only %s 2>&1
+
+static_assert(my_pi == (double)3.1415926535897932385L);
+static_assert(my_pi == '3');
+
+static_assert(Wrapper::my_const == 1.f);
+static_assert(Wrapper::my_const == nullptr);
+static_assert(Wrapper::my_const == a);
Index: test/ASTMerge/var-cpp/Inputs/var1.cpp
===
--- /dev/null
+++ test/ASTMerge/var-cpp/Inputs/var1.cpp
@@ -0,0 +1,17 @@
+
+template
+constexpr T my_pi = T(3.1415926535897932385L); // variable template
+
+template <> constexpr char my_pi = '3'; // variable template specialization
+
+template
+struct Wrapper {
+ template static constexpr U my_const = U(1);
+ // Variable template partial specialization with member variable.
+ template static constexpr U *my_const = (U *)(0);
+};
+
+constexpr char a[] = "hello";
+
+template <> template <>
+constexpr const char *Wrapper::my_const = a;
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -23,7 +23,6 @@
#include "clang/Basic/FileManager.h"
#include "clang/Basic/SourceManager.h"
#include "llvm/Support/MemoryBuffer.h"
-#include
namespace clang {
class ASTNodeImporter : public TypeVisitor,
@@ -1335,6 +1334,21 @@
return false;
}
+template <>
+bool ASTNodeImporter::ImportTemplateArgumentListInfo(
+const TemplateArgumentListInfo &From, TemplateArgumentListInfo &Result) {
+ return ImportTemplateArgumentListInfo(
+ From.getLAngleLoc(), From.getRAngleLoc(), From.arguments(), Result);
+}
+
+template <>
+bool ASTNodeImporter::ImportTemplateArgumentListInfo<
+ASTTemplateArgumentListInfo>(const ASTTemplateArgumentListInfo &From,
+ TemplateArgumentListInfo &Result) {
+ return ImportTemplateArgumentListInfo(From.LAngleLoc, From.RAngleLoc,
+From.arguments(), Result);
+}
+
bool ASTNodeImporter::IsStructuralMatch(RecordDecl *FromRecord,
RecordDecl *ToRecord, bool Complain) {
// Eliminate a potential failure point where we attempt to re-import
@@ -1655,10 +1669,8 @@
SourceLocation StartL = Importer.Import(D->getLocStart());
TypedefNameDecl *ToTypedef;
if (IsAlias)
-ToTypedef = TypeAliasDecl::Create(Importer.getToContext(), DC,
- StartL, Loc,
- Name.
[PATCH] D43074: [Analyzer] Fix a typo about `categories::MemoryError` in `MallocChecker.cpp`
a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D43074 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43012: [ASTImporter] Fix lexical DC for templated decls; support VarTemplatePartialSpecDecl
a.sidorin marked 2 inline comments as done. a.sidorin added inline comments. Comment at: lib/AST/ASTImporter.cpp:4296 // Create the declaration that is being templated. - SourceLocation StartLoc = Importer.Import(DTemplated->getLocStart()); - SourceLocation IdLoc = Importer.Import(DTemplated->getLocation()); - TypeSourceInfo *TInfo = Importer.Import(DTemplated->getTypeSourceInfo()); - VarDecl *D2Templated = VarDecl::Create(Importer.getToContext(), DC, StartLoc, - IdLoc, Name.getAsIdentifierInfo(), T, - TInfo, DTemplated->getStorageClass()); - D2Templated->setAccess(DTemplated->getAccess()); - D2Templated->setQualifierInfo(Importer.Import(DTemplated->getQualifierLoc())); - D2Templated->setLexicalDeclContext(LexicalDC); - - // Importer.Imported(DTemplated, D2Templated); - // LexicalDC->addDeclInternal(D2Templated); - - // Merge the initializer. - if (ImportDefinition(DTemplated, D2Templated)) + VarDecl *ToTemplated = dyn_cast_or_null(Importer.Import(DTemplated)); + if (!ToTemplated) xazax.hun wrote: > `auto *` to not repeat type. I usually prefer to keep the type if it doesn't give a large space win because it hurts readability a bit. From `VarDecl *`, we can instantly find the type; for `auto`, we have to look forward. (Yes, VarTemplatePartialSpecializationDecl has to be replaced immediately :) ). Other issue is that QtCreator I use for development still doesn't have an autocompletion for auto types. However, LLVM says: "do use auto with initializers like cast(...)", so I'll change this. Repository: rC Clang https://reviews.llvm.org/D43012 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43012: [ASTImporter] Fix lexical DC for templated decls; support VarTemplatePartialSpecDecl
a.sidorin updated this revision to Diff 133626.
a.sidorin marked an inline comment as done.
a.sidorin added a comment.
Fix style issues found on review.
Repository:
rC Clang
https://reviews.llvm.org/D43012
Files:
lib/AST/ASTImporter.cpp
test/ASTMerge/var-cpp/Inputs/var1.cpp
test/ASTMerge/var-cpp/test.cpp
unittests/AST/ASTImporterTest.cpp
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -542,20 +542,32 @@
TEST(ImportType, ImportTypeAliasTemplate) {
MatchVerifier Verifier;
- testImport("template "
- "struct dummy { static const int i = K; };"
- "template using dummy2 = dummy;"
- "int declToImport() { return dummy2<3>::i; }",
- Lang_CXX11, "", Lang_CXX11, Verifier,
- functionDecl(
- hasBody(
- compoundStmt(
- has(
- returnStmt(
- has(
- implicitCastExpr(
- has(
- declRefExpr());
+ testImport(
+ "template "
+ "struct dummy { static const int i = K; };"
+ "template using dummy2 = dummy;"
+ "int declToImport() { return dummy2<3>::i; }",
+ Lang_CXX11, "", Lang_CXX11, Verifier,
+ functionDecl(
+ hasBody(compoundStmt(
+ has(returnStmt(has(implicitCastExpr(has(declRefExpr(,
+ unless(hasAncestor(translationUnitDecl(has(typeAliasDecl()));
+}
+
+const internal::VariadicDynCastAllOfMatcher
+varTemplateSpecializationDecl;
+
+TEST(ImportDecl, ImportVarTemplate) {
+ MatchVerifier Verifier;
+ testImport(
+ "template "
+ "T pi = T(3.1415926535897932385L);"
+ "void declToImport() { pi; }",
+ Lang_CXX11, "", Lang_CXX11, Verifier,
+ functionDecl(
+ hasBody(has(declRefExpr(to(varTemplateSpecializationDecl(),
+ unless(hasAncestor(translationUnitDecl(has(varDecl(
+ hasName("pi"), unless(varTemplateSpecializationDecl();
}
TEST(ImportType, ImportPackExpansion) {
Index: test/ASTMerge/var-cpp/test.cpp
===
--- /dev/null
+++ test/ASTMerge/var-cpp/test.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -emit-pch -std=c++17 -o %t.1.ast %S/Inputs/var1.cpp
+// RUN: %clang_cc1 -std=c++17 -ast-merge %t.1.ast -fsyntax-only %s 2>&1
+
+static_assert(my_pi == (double)3.1415926535897932385L);
+static_assert(my_pi == '3');
+
+static_assert(Wrapper::my_const == 1.f);
+static_assert(Wrapper::my_const == nullptr);
+static_assert(Wrapper::my_const == a);
Index: test/ASTMerge/var-cpp/Inputs/var1.cpp
===
--- /dev/null
+++ test/ASTMerge/var-cpp/Inputs/var1.cpp
@@ -0,0 +1,17 @@
+
+template
+constexpr T my_pi = T(3.1415926535897932385L); // variable template
+
+template <> constexpr char my_pi = '3'; // variable template specialization
+
+template
+struct Wrapper {
+ template static constexpr U my_const = U(1);
+ // Variable template partial specialization with member variable.
+ template static constexpr U *my_const = (U *)(0);
+};
+
+constexpr char a[] = "hello";
+
+template <> template <>
+constexpr const char *Wrapper::my_const = a;
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -23,7 +23,6 @@
#include "clang/Basic/FileManager.h"
#include "clang/Basic/SourceManager.h"
#include "llvm/Support/MemoryBuffer.h"
-#include
namespace clang {
class ASTNodeImporter : public TypeVisitor,
@@ -1335,6 +1334,21 @@
return false;
}
+template <>
+bool ASTNodeImporter::ImportTemplateArgumentListInfo(
+const TemplateArgumentListInfo &From, TemplateArgumentListInfo &Result) {
+ return ImportTemplateArgumentListInfo(
+ From.getLAngleLoc(), From.getRAngleLoc(), From.arguments(), Result);
+}
+
+template <>
+bool ASTNodeImporter::ImportTemplateArgumentListInfo<
+ASTTemplateArgumentListInfo>(const ASTTemplateArgumentListInfo &From,
+ TemplateArgumentListInfo &Result) {
+ return ImportTemplateArgumentListInfo(From.LAngleLoc, From.RAngleLoc,
+From.arguments(), Result);
+}
+
bool ASTNodeImporter::IsStructuralMatch(RecordDecl *FromRecord,
RecordDecl *ToRecord, bool Complain) {
// Eliminate a potential failure point where we attempt to re-import
@@ -1655,10 +1669,8 @@
SourceLocation StartL = Importer.Import(D->getLocStart());
TypedefNameDecl *ToTypedef;
if (IsAlias)
-ToTypedef = TypeAliasDecl::Create(Importer.getToContext(), DC,
- StartL, Loc,
- Name.getAsIdentifierInfo(),
-
[PATCH] D43012: [ASTImporter] Fix lexical DC for templated decls; support VarTemplatePartialSpecDecl
a.sidorin added inline comments. Comment at: lib/AST/ASTImporter.cpp:2858 + + // Templated declarations should never appear in the enclosing DeclContext. + if (!D->getDescribedVarTemplate()) martong wrote: > In case of class templates, the explicit instantiation is the member of the > DeclContext. It does not belong to the DeclContext only in case of implicit > instantiations. > I suppose the same is true for template variables as well. > In our code base we fixed it by checking on the TSK_ kind, see > https://github.com/Ericsson/clang/pull/270/files. > > @xazax.hun perhaps we should open source some of our fixes as well? This code handles templated declarations, not template instantiations. Every TemplateDecl has an underlying templated declaration that is used while instantiating the template; it is not the same as template specialization. And it is not visible from the DeclContext it refers to via lookup. Or am I misunderstanding something? Yes, the issue with implicit instantiations still persists but it is not the target for this patch. I have took a look at the patch; it looks like it fixes a separate issue (and I welcome you to post the patch here!). Repository: rC Clang https://reviews.llvm.org/D43012 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43012: [ASTImporter] Fix lexical DC for templated decls; support VarTemplatePartialSpecDecl
This revision was automatically updated to reflect the committed changes.
Closed by commit rL325116: [ASTImporter] Fix lexical DC for templated decls;
support… (authored by a.sidorin, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D43012?vs=133626&id=134187#toc
Repository:
rL LLVM
https://reviews.llvm.org/D43012
Files:
cfe/trunk/lib/AST/ASTImporter.cpp
cfe/trunk/test/ASTMerge/var-cpp/Inputs/var1.cpp
cfe/trunk/test/ASTMerge/var-cpp/test.cpp
cfe/trunk/unittests/AST/ASTImporterTest.cpp
Index: cfe/trunk/test/ASTMerge/var-cpp/Inputs/var1.cpp
===
--- cfe/trunk/test/ASTMerge/var-cpp/Inputs/var1.cpp
+++ cfe/trunk/test/ASTMerge/var-cpp/Inputs/var1.cpp
@@ -0,0 +1,17 @@
+
+template
+constexpr T my_pi = T(3.1415926535897932385L); // variable template
+
+template <> constexpr char my_pi = '3'; // variable template specialization
+
+template
+struct Wrapper {
+ template static constexpr U my_const = U(1);
+ // Variable template partial specialization with member variable.
+ template static constexpr U *my_const = (U *)(0);
+};
+
+constexpr char a[] = "hello";
+
+template <> template <>
+constexpr const char *Wrapper::my_const = a;
Index: cfe/trunk/test/ASTMerge/var-cpp/test.cpp
===
--- cfe/trunk/test/ASTMerge/var-cpp/test.cpp
+++ cfe/trunk/test/ASTMerge/var-cpp/test.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -emit-pch -std=c++17 -o %t.1.ast %S/Inputs/var1.cpp
+// RUN: %clang_cc1 -std=c++17 -ast-merge %t.1.ast -fsyntax-only %s 2>&1
+
+static_assert(my_pi == (double)3.1415926535897932385L);
+static_assert(my_pi == '3');
+
+static_assert(Wrapper::my_const == 1.f);
+static_assert(Wrapper::my_const == nullptr);
+static_assert(Wrapper::my_const == a);
Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -23,7 +23,6 @@
#include "clang/Basic/FileManager.h"
#include "clang/Basic/SourceManager.h"
#include "llvm/Support/MemoryBuffer.h"
-#include
namespace clang {
class ASTNodeImporter : public TypeVisitor,
@@ -1335,6 +1334,21 @@
return false;
}
+template <>
+bool ASTNodeImporter::ImportTemplateArgumentListInfo(
+const TemplateArgumentListInfo &From, TemplateArgumentListInfo &Result) {
+ return ImportTemplateArgumentListInfo(
+ From.getLAngleLoc(), From.getRAngleLoc(), From.arguments(), Result);
+}
+
+template <>
+bool ASTNodeImporter::ImportTemplateArgumentListInfo<
+ASTTemplateArgumentListInfo>(const ASTTemplateArgumentListInfo &From,
+ TemplateArgumentListInfo &Result) {
+ return ImportTemplateArgumentListInfo(From.LAngleLoc, From.RAngleLoc,
+From.arguments(), Result);
+}
+
bool ASTNodeImporter::IsStructuralMatch(RecordDecl *FromRecord,
RecordDecl *ToRecord, bool Complain) {
// Eliminate a potential failure point where we attempt to re-import
@@ -1655,10 +1669,8 @@
SourceLocation StartL = Importer.Import(D->getLocStart());
TypedefNameDecl *ToTypedef;
if (IsAlias)
-ToTypedef = TypeAliasDecl::Create(Importer.getToContext(), DC,
- StartL, Loc,
- Name.getAsIdentifierInfo(),
- TInfo);
+ToTypedef = TypeAliasDecl::Create(Importer.getToContext(), DC, StartL, Loc,
+ Name.getAsIdentifierInfo(), TInfo);
else
ToTypedef = TypedefDecl::Create(Importer.getToContext(), DC,
StartL, Loc,
@@ -1668,7 +1680,11 @@
ToTypedef->setAccess(D->getAccess());
ToTypedef->setLexicalDeclContext(LexicalDC);
Importer.Imported(D, ToTypedef);
- LexicalDC->addDeclInternal(ToTypedef);
+
+ // Templated declarations should not appear in DeclContext.
+ TypeAliasDecl *FromAlias = IsAlias ? cast(D) : nullptr;
+ if (!FromAlias || !FromAlias->getDescribedAliasTemplate())
+LexicalDC->addDeclInternal(ToTypedef);
return ToTypedef;
}
@@ -1686,11 +1702,11 @@
DeclContext *DC, *LexicalDC;
DeclarationName Name;
SourceLocation Loc;
- NamedDecl *ToD;
- if (ImportDeclParts(D, DC, LexicalDC, Name, ToD, Loc))
+ NamedDecl *FoundD;
+ if (ImportDeclParts(D, DC, LexicalDC, Name, FoundD, Loc))
return nullptr;
- if (ToD)
-return ToD;
+ if (FoundD)
+return FoundD;
// If this typedef is not in block scope, determine whether we've
// seen a typedef with the same name (that we can merge with) or any
@@ -1723,19 +1739,21 @@
if (!Params)
return nullptr;
- NamedDecl *TemplDecl = cast_or_null(
+ auto *TemplDecl = cast_or_null(
Importer.Import(D->getTemplatedDecl()));
if (!TemplDecl)
return nullptr;
TypeAliasTemplat
[PATCH] D42645: New simple Checker for mmap calls
a.sidorin added a comment. Hello David, There are still some items for improvement. However, if we move this checker into 'alpha' category, as Artem pointed, I think it can be accepted for merge in its current state and improved later. Repository: rC Clang https://reviews.llvm.org/D42645 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases.
a.sidorin accepted this revision.
a.sidorin added a comment.
Thank you! Just some of nits inline.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:394
+
+bool ExprEngine::areTemporaryMaterializationsClear(
+ProgramStateRef State, const LocationContext *FromLC,
```
for (const auto &I : State->get()) {
auto *LCtx = I.first.second;
if (LCtx == FromLC || (LCtx->isParentOf(From) && (!To ||
To->isParentOf(LCtx)))
return false;
}
return true;
```
is a bit shorter but less evident so I won't insist.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:396
+ProgramStateRef State, const LocationContext *FromLC,
+const LocationContext *ToLC) {
+ const LocationContext *LC = FromLC;
EndLC? (similar to iterators)
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:400
+assert(LC && "ToLC must be a parent of FromLC!");
+for (auto I : State->get())
+ if (I.first.second == LC)
const auto &?
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:503
+Key.first->printPretty(Out, nullptr, PP);
+if (Value)
+ Out << " : " << Value;
As I see from line 350, `Value` is always non-null. And there is same check on
line 659. Am I missing something?
https://reviews.llvm.org/D43497
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43533: [CFG] [analyzer] NFC: Refactor ConstructionContext into a finite set of cases.
a.sidorin accepted this revision. a.sidorin added inline comments. Comment at: include/clang/Analysis/ConstructionContext.h:119 + static const ConstructionContext * + finalize(BumpVectorContext &C, const ConstructionContextLayer *TopLayer); + Maybe just `build()`? For me, `finalize()` strongly associates with Java's broken clean-up mechanism. https://reviews.llvm.org/D43533 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42645: New simple Checker for mmap calls
a.sidorin accepted this revision. a.sidorin added a comment. Looks good to me as an 'alpha' checker. Thank you David! I'd prefer this patch to be accepted by somebody else as well before committing it. https://reviews.llvm.org/D42645 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis
a.sidorin accepted this revision. a.sidorin added a comment. Hello Daniel & Gabor, Thank you very much for your work. This patch looks good to me but I think such a change should also be approved by maintainers. https://reviews.llvm.org/D30691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32751: [ASTImporter] Support new kinds of declarations (mostly Using*)
a.sidorin added a comment. Hello Peter, `if (!ToDecl) return nullptr;` is used if original node is always non-null. `if(!ToDecl && FromDecl) return nullptr;` is used if original node can be null. If the imported node is null, the result of import is null as well so such import is OK. `ObjectXY::Create(...Import(FromDecl))` is often used for source locations - as I guess, invalid source location is OK usually. It can also be used if we know that node should already be imported, but usually indicates a potential error. https://reviews.llvm.org/D32751 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32751: [ASTImporter] Support new kinds of declarations (mostly Using*)
a.sidorin updated this revision to Diff 102679.
a.sidorin marked an inline comment as done.
a.sidorin added a comment.
Herald added a subscriber: kristof.beyls.
Add a FIXME.
https://reviews.llvm.org/D32751
Files:
lib/AST/ASTImporter.cpp
test/ASTMerge/namespace/Inputs/namespace1.cpp
test/ASTMerge/namespace/Inputs/namespace2.cpp
test/ASTMerge/namespace/test.cpp
Index: test/ASTMerge/namespace/test.cpp
===
--- test/ASTMerge/namespace/test.cpp
+++ test/ASTMerge/namespace/test.cpp
@@ -1,6 +1,17 @@
-// RUN: %clang_cc1 -emit-pch -o %t.1.ast %S/Inputs/namespace1.cpp
-// RUN: %clang_cc1 -emit-pch -o %t.2.ast %S/Inputs/namespace2.cpp
-// RUN: not %clang_cc1 -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -emit-pch -std=c++1z -o %t.1.ast %S/Inputs/namespace1.cpp
+// RUN: %clang_cc1 -emit-pch -std=c++1z -o %t.2.ast %S/Inputs/namespace2.cpp
+// RUN: not %clang_cc1 -std=c++1z -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only %s 2>&1 | FileCheck %s
+
+static_assert(TestAliasName::z == 4);
+static_assert(ContainsInline::z == 10);
+
+void testImport() {
+ typedef TestUnresolvedTypenameAndValueDecls::Derived Imported;
+ Imported a; // Successfull instantiation
+ static_assert(sizeof(Imported::foo) == sizeof(int));
+ static_assert(sizeof(TestUnresolvedTypenameAndValueDecls::Derived::NewUnresolvedUsingType) == sizeof(double));
+}
+
// CHECK: namespace2.cpp:16:17: error: external variable 'z' declared with incompatible types in different translation units ('double' vs. 'float')
// CHECK: namespace1.cpp:16:16: note: declared here with type 'float'
Index: test/ASTMerge/namespace/Inputs/namespace2.cpp
===
--- test/ASTMerge/namespace/Inputs/namespace2.cpp
+++ test/ASTMerge/namespace/Inputs/namespace2.cpp
@@ -15,3 +15,46 @@
namespace N3 {
extern double z;
}
+
+namespace Enclosing {
+namespace Nested {
+ const int z = 4;
+}
+}
+
+namespace ContainsInline {
+ inline namespace Inline {
+const int z = 10;
+ }
+}
+
+namespace TestAliasName = Enclosing::Nested;
+// NOTE: There is no warning on this alias.
+namespace AliasWithSameName = Enclosing::Nested;
+
+namespace TestUsingDecls {
+
+namespace A {
+void foo();
+}
+namespace B {
+using A::foo; // <- a UsingDecl creating a UsingShadow
+}
+
+}// end namespace TestUsingDecls
+
+namespace TestUnresolvedTypenameAndValueDecls {
+
+template class Base;
+template class Derived : public Base {
+public:
+ using typename Base::foo;
+ using Base::bar;
+ typedef typename Derived::foo NewUnresolvedUsingType;
+};
+
+} // end namespace TestUnresolvedTypenameAndValueDecls
+
+namespace TestUsingNamespace {
+ using namespace Enclosing;
+}
Index: test/ASTMerge/namespace/Inputs/namespace1.cpp
===
--- test/ASTMerge/namespace/Inputs/namespace1.cpp
+++ test/ASTMerge/namespace/Inputs/namespace1.cpp
@@ -15,3 +15,13 @@
namespace N3 {
extern float z;
}
+
+namespace AliasWithSameName = N3;
+
+namespace TestUnresolvedTypenameAndValueDecls {
+template class Base {
+public:
+ typedef T foo;
+ void bar();
+};
+}
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -58,7 +58,7 @@
QualType VisitExtVectorType(const ExtVectorType *T);
QualType VisitFunctionNoProtoType(const FunctionNoProtoType *T);
QualType VisitFunctionProtoType(const FunctionProtoType *T);
-// FIXME: UnresolvedUsingType
+QualType VisitUnresolvedUsingType(const UnresolvedUsingType *T);
QualType VisitParenType(const ParenType *T);
QualType VisitTypedefType(const TypedefType *T);
QualType VisitTypeOfExprType(const TypeOfExprType *T);
@@ -128,8 +128,8 @@
TemplateParameterList *ImportTemplateParameterList(
TemplateParameterList *Params);
TemplateArgument ImportTemplateArgument(const TemplateArgument &From);
-TemplateArgumentLoc ImportTemplateArgumentLoc(
-const TemplateArgumentLoc &TALoc, bool &Error);
+Optional ImportTemplateArgumentLoc(
+const TemplateArgumentLoc &TALoc);
bool ImportTemplateArguments(const TemplateArgument *FromArgs,
unsigned NumFromArgs,
SmallVectorImpl &ToArgs);
@@ -142,10 +142,12 @@
bool IsStructuralMatch(ClassTemplateDecl *From, ClassTemplateDecl *To);
bool IsStructuralMatch(VarTemplateDecl *From, VarTemplateDecl *To);
Decl *VisitDecl(Decl *D);
+Decl *VisitEmptyDecl(EmptyDecl *D);
Decl *VisitAccessSpecDecl(AccessSpecDecl *D);
Decl *VisitStaticAssertDecl(StaticAssertDecl *D);
Decl *VisitTranslationUnitDecl(TranslationUnitDecl *D);
Decl *VisitNamespaceDecl(NamespaceDecl *D);
+Decl *VisitNamespaceAliasDecl(NamespaceAli
[PATCH] D32751: [ASTImporter] Support new kinds of declarations (mostly Using*)
a.sidorin added inline comments. Comment at: lib/AST/ASTImporter.cpp:2993 + return nullptr; + } + szepet wrote: > nit: As I see these cases typically handled in the way: > > ``` > FrPattern = .; > ToPattern = ..; > if(FrPattern && !ToPattern) > ``` > Just to avoid the nested ifstmt. The logic is a bit more complicated. There are 3 cases: # Both `FromPattern` and `ToPattern` are `nullptr`s. Just continue. # `FromPattern` is non-null and `ToPattern` is null. Return error (`nullptr`). # Both `FromPattern` and `ToPattern` are `nullptr`s. Do the `set...` action. So, it will require nested `if`s or a code like: ``` if (FromPattern && ToPattern) set... if (FromPattern && !ToPattern) return nullptr; ``` Comment at: lib/AST/ASTImporter.cpp:3000 +else + // FIXME: We return a nullptr here but the definition is already created + // and available with lookups. How to fix this?.. szepet wrote: > I dont see the problem with moving these up , collect nad investigate them in > a smallset before the Create function, then adding them to the created > ToUsing decl. It could be done as a follow up patch, dont want to mark it as > a blocking issue. There is a chicken and egg problem: both UsingShadowDecl and UsingDecl reference each other and UsingShadowDecl gets referenced UsingDecl as a ctor argument. If you have a good idea on how to resolve this dependency correctly, please point me. https://reviews.llvm.org/D32751 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34277: [analyzer] Bump default performance thresholds?
a.sidorin added a comment. Hi Artem, Could you tell what code bases did you use to collect your statistics? I'll try to check the patch on our code bases. I think we should be careful about default settings. Maybe we should introduce another UMK_* for deeper analysis instead? https://reviews.llvm.org/D34277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D6550: ASTImporter: Fix missing SourceLoc imports
a.sidorin added a comment. Will anybody object if I commit this change without a test? This bug seems to be pretty obvious but, unfortunately, I'm not familiar with Objective C. https://reviews.llvm.org/D6550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D6550: ASTImporter: Fix missing SourceLoc imports
a.sidorin added a comment. Thank you Sean, I'll try. https://reviews.llvm.org/D6550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34277: [analyzer] Bump default performance thresholds?
a.sidorin accepted this revision. a.sidorin added a comment. Ok, I hope this will work. Anyway, we can always revert this patch in case of any problems. https://reviews.llvm.org/D34277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D16403: Add scope information to CFG
a.sidorin added a comment. Hi Matthias, I have posted a comment about review duplication (more than a year ago!) in your review but you haven't answered. So, all this time we were thinking that you do separate non-related work. @dcoughlin As a reviewer of both patches - could you tell us what's the difference between them? And how are we going to resolve this issue? Repository: rL LLVM https://reviews.llvm.org/D16403 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38694: [ASTImporter] Support importing CXXUnresolvedConstructExpr and UnresolvedLookupExpr
a.sidorin updated this revision to Diff 127283.
a.sidorin added a comment.
Fixed sanity check.
Repository:
rC Clang
https://reviews.llvm.org/D38694
Files:
lib/AST/ASTImporter.cpp
unittests/AST/ASTImporterTest.cpp
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -653,5 +653,39 @@
usingShadowDecl();
}
+TEST(ImportExpr, ImportUnresolvedLookupExpr) {
+ MatchVerifier Verifier;
+ EXPECT_TRUE(testImport("template int foo();"
+ "template void declToImport() {"
+ " ::foo;"
+ " ::template foo;"
+ "}",
+ Lang_CXX, "", Lang_CXX, Verifier,
+ functionTemplateDecl(has(functionDecl(has(
+ compoundStmt(has(unresolvedLookupExpr();
+}
+
+TEST(ImportExpr, ImportCXXUnresolvedConstructExpr) {
+ MatchVerifier Verifier;
+ EXPECT_TRUE(
+ testImport("template class C { T t; };"
+ "template void declToImport() {"
+ " C d;"
+ " d.t = T();"
+ "}",
+ Lang_CXX, "", Lang_CXX, Verifier,
+ functionTemplateDecl(has(functionDecl(has(compoundStmt(has(
+ binaryOperator(has(cxxUnresolvedConstructExpr()));
+ EXPECT_TRUE(
+ testImport("template class C { T t; };"
+ "template void declToImport() {"
+ " C d;"
+ " (&d)->t = T();"
+ "}",
+ Lang_CXX, "", Lang_CXX, Verifier,
+ functionTemplateDecl(has(functionDecl(has(compoundStmt(has(
+ binaryOperator(has(cxxUnresolvedConstructExpr()));
+}
+
} // end namespace ast_matchers
} // end namespace clang
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -287,6 +287,8 @@
Expr *VisitCXXConstructExpr(CXXConstructExpr *E);
Expr *VisitCXXMemberCallExpr(CXXMemberCallExpr *E);
Expr *VisitCXXDependentScopeMemberExpr(CXXDependentScopeMemberExpr *E);
+Expr *VisitCXXUnresolvedConstructExpr(CXXUnresolvedConstructExpr *CE);
+Expr *VisitUnresolvedLookupExpr(UnresolvedLookupExpr *E);
Expr *VisitExprWithCleanups(ExprWithCleanups *EWC);
Expr *VisitCXXThisExpr(CXXThisExpr *E);
Expr *VisitCXXBoolLiteralExpr(CXXBoolLiteralExpr *E);
@@ -5885,6 +5887,65 @@
cast_or_null(ToFQ), MemberNameInfo, ResInfo);
}
+Expr *ASTNodeImporter::VisitCXXUnresolvedConstructExpr(
+CXXUnresolvedConstructExpr *CE) {
+
+ unsigned NumArgs = CE->arg_size();
+
+ llvm::SmallVector ToArgs(NumArgs);
+ if (ImportArrayChecked(CE->arg_begin(), CE->arg_end(), ToArgs.begin()))
+return nullptr;
+
+ return CXXUnresolvedConstructExpr::Create(
+ Importer.getToContext(), Importer.Import(CE->getTypeSourceInfo()),
+ Importer.Import(CE->getLParenLoc()), llvm::makeArrayRef(ToArgs),
+ Importer.Import(CE->getRParenLoc()));
+}
+
+Expr *ASTNodeImporter::VisitUnresolvedLookupExpr(UnresolvedLookupExpr *E) {
+ CXXRecordDecl *NamingClass =
+ cast_or_null(Importer.Import(E->getNamingClass()));
+ if (E->getNamingClass() && !NamingClass)
+return nullptr;
+
+ DeclarationName Name = Importer.Import(E->getName());
+ if (E->getName() && !Name)
+return nullptr;
+
+ DeclarationNameInfo NameInfo(Name, Importer.Import(E->getNameLoc()));
+ // Import additional name location/type info.
+ ImportDeclarationNameLoc(E->getNameInfo(), NameInfo);
+
+ UnresolvedSet<8> ToDecls;
+ for (Decl *D : E->decls()) {
+if (NamedDecl *To = cast_or_null(Importer.Import(D)))
+ ToDecls.addDecl(To);
+else
+ return nullptr;
+ }
+
+ TemplateArgumentListInfo ToTAInfo(Importer.Import(E->getLAngleLoc()),
+Importer.Import(E->getRAngleLoc()));
+ TemplateArgumentListInfo *ResInfo = nullptr;
+ if (E->hasExplicitTemplateArgs()) {
+if (ImportTemplateArgumentListInfo(E->template_arguments(), ToTAInfo))
+ return nullptr;
+ResInfo = &ToTAInfo;
+ }
+
+ if (ResInfo || E->getTemplateKeywordLoc().isValid())
+return UnresolvedLookupExpr::Create(
+Importer.getToContext(), NamingClass,
+Importer.Import(E->getQualifierLoc()),
+Importer.Import(E->getTemplateKeywordLoc()), NameInfo, E->requiresADL(),
+ResInfo, ToDecls.begin(), ToDecls.end());
+
+ return UnresolvedLookupExpr::Create(
+ Importer.getToContext(), NamingClass,
+ Importer.Import(E->getQualifierLoc()), NameInfo, E->requiresADL(),
+ E->isOverloaded(), ToDecls.begin(), ToDecls.end());
+}
+
Expr *ASTNodeImporter::VisitCallExpr(CallExpr *E) {
QualType T = Importer.Import(E->getType());
if (T.isNull())
___
[PATCH] D19979: [analyzer] ScopeContext - initial implementation
a.sidorin added a comment. This patch still depends on scope implementation in CFG. There is no final implementation; after initial implementation is done, I'll update the patch. https://reviews.llvm.org/D19979 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41151: [analyzer] Adding LoopContext and improve loop modeling
a.sidorin added a comment. This thing is very similar to https://reviews.llvm.org/D19979. Do we really need to create a separate LoopContext or we can reuse ScopeContext instead? https://reviews.llvm.org/D41151 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40560: [analyzer] WIP: Get construction into `operator new` running in simple cases.
a.sidorin added a comment. Hi Artem. This patch looks OK, just stylish issues. Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:112 + // It means that we cannot handle construction into null or garbage pointers. + // Such cosntructors need to be handled by checkers to ensure that a warning + // is displayed to the user and that analysis doesn't explore such paths constructors Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:474 + StmtNodeBuilder CallBldr(DstPreCall, DstPostCall, *currBldrCtx); + for (auto I: DstPreCall) +defaultEvalCall(CallBldr, I, *Call); Space after ':'? (Same below and upper) Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:522 +symVal = peekCXXNewAllocatorValue(State); + State = popCXXNewAllocatorValue(State); + Should this be under 'if' as well? https://reviews.llvm.org/D40560 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D6550: ASTImporter: Fix missing SourceLoc imports
a.sidorin updated this revision to Diff 127375.
a.sidorin added reviewers: xazax.hun, szepet.
a.sidorin added a comment.
Herald added a subscriber: rnkovacs.
Removed already fixed stuff, added a test for remaining.
Repository:
rC Clang
https://reviews.llvm.org/D6550
Files:
lib/AST/ASTImporter.cpp
test/ASTMerge/interface/Inputs/interface1.m
Index: test/ASTMerge/interface/Inputs/interface1.m
===
--- test/ASTMerge/interface/Inputs/interface1.m
+++ test/ASTMerge/interface/Inputs/interface1.m
@@ -100,4 +100,6 @@
@implementation I15 : I12
@end
-
+@interface ImportSelectorSLoc { }
+-(int)addInt:(int)a toInt:(int)b moduloInt:(int)c; // don't crash here
+@end
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2857,9 +2857,13 @@
ToParams[I]->setOwningFunction(ToMethod);
ToMethod->addDeclInternal(ToParams[I]);
}
+
SmallVector SelLocs;
D->getSelectorLocs(SelLocs);
- ToMethod->setMethodParams(Importer.getToContext(), ToParams, SelLocs);
+ for (SourceLocation &Loc : SelLocs)
+Loc = Importer.Import(Loc);
+
+ ToMethod->setMethodParams(Importer.getToContext(), ToParams, SelLocs);
ToMethod->setLexicalDeclContext(LexicalDC);
Importer.Imported(D, ToMethod);
Index: test/ASTMerge/interface/Inputs/interface1.m
===
--- test/ASTMerge/interface/Inputs/interface1.m
+++ test/ASTMerge/interface/Inputs/interface1.m
@@ -100,4 +100,6 @@
@implementation I15 : I12
@end
-
+@interface ImportSelectorSLoc { }
+-(int)addInt:(int)a toInt:(int)b moduloInt:(int)c; // don't crash here
+@end
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2857,9 +2857,13 @@
ToParams[I]->setOwningFunction(ToMethod);
ToMethod->addDeclInternal(ToParams[I]);
}
+
SmallVector SelLocs;
D->getSelectorLocs(SelLocs);
- ToMethod->setMethodParams(Importer.getToContext(), ToParams, SelLocs);
+ for (SourceLocation &Loc : SelLocs)
+Loc = Importer.Import(Loc);
+
+ ToMethod->setMethodParams(Importer.getToContext(), ToParams, SelLocs);
ToMethod->setLexicalDeclContext(LexicalDC);
Importer.Imported(D, ToMethod);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41077: [analyser] different.CallArgsOrder checker implementation
a.sidorin added a comment.
Hi Alexey,
This commit strongly needs testing on some real code. I cannot predict the TP
rate of this checker now.
Regarding implementation, you can find some remarks inline.
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:31
+
+class ParamsGroup {
+public:
We have a class without any private parts. This means that we should declare it
as 'struct' or do some redesign - the way it is constructed (with expressions
like `PGL.back().ParamsVec.push_back(std::move(p));` clearly indicates some
design issues. I suggest at least adding a wrapper above ParamsGroupVec with
methods:
`ParamGroup &addParam(const ParmVarDecl *ParamVD)`; // adds a variable into
existing group or creates new one
`void reclaimLastGroupIfSingle(); // deletes last added group if it has only a
single element`
and moving some logic here.
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:33
+public:
+ ParamsGroup() : StartIndex(0) { ParamsVec.reserve(4); }
+
You don't need to reserve the amount of items same or less then SmallVector
default size - they are already stack-allocated.
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:112
+ checkCallExpr(CE);
+if (const CXXConstructExpr *CE =
+Result.Nodes.getNodeAs("cxxConstructExpr"))
else if?
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:119
+
+StringRefVec ParamsGroup::trimSame(StringRefVec StrVec) {
+ return rtrimSame(ltrimSame(StrVec));
trim* functions should be moved out of ParamsGroup because they are not
related.
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:139
+
+if (SamePrefixSize > StrSize) {
+ SamePrefix = SamePrefix.drop_back(SamePrefixSize - StrSize);
We can the code:
```
SamePrefix = SamePrefix.take_front(StrSize);
SamePrefixSize = SamePrefix.size();
```
instead. The behaviour of `StringRef::take_front(size_t N)` is well-defined if
N >= size().
Same below: drop_front() can be replaced with take_back().
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:144
+
+for (size_t i = 0; i < SamePrefixSize; i++) {
+ if (Str[i] != SamePrefix[i]) {
What you are trying to do here (find common prefix of two strings) can be done
easier using std::mismatch().
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:157
+ const size_t SamePrefixSize = SamePrefix.size();
+ std::transform(StrVec.begin(), StrVec.end(), StrVec.begin(),
+ [SamePrefixSize](const StringRef &Str) {
llvm::transform(StrVec, StrVec.begin(), ...)
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:164
+
+StringRefVec ParamsGroup::rtrimSame(StringRefVec StrVec) {
+ if (StrVec.empty())
szepet wrote:
> Please add a comment to this function which describes its input, output,
> purpose.
With names like removeCommon[Pre/Suf]fix, the intention of these functions will
be much cleaner. Also, please do not pass vectors by value.
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:185
+
+for (size_t i = StrSize, j = SameSuffixSize; i > 0 && j > 0; i--, j--) {
+ if (Str[i - 1] != SameSuffix[j - 1]) {
We can use std::mismatch with std::reverse_iterator (std::rbegin(),
std::rend()).
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:208
+CallArgsOrderChecker::getOrBuildParamsGroups(const FunctionDecl *FD) const {
+ const auto It = PGLCache.find(FD);
+ if (It != PGLCache.end())
We can use `try_emplace()` to construct the new item in-place.
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:215
+ const unsigned NumParams = FD->getNumParams();
+ if (NumParams > 0) {
+const ParmVarDecl *prevParam = FD->getParamDecl(0);
This is already check in the caller. I think, this can be replaced with an
assertion.
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:257
+
+static StringRef getExprName(const Expr *E) {
+ const Expr *OrigE = E->IgnoreParenCasts();
Could you please explain what kind of ExprName are you trying to get?
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:259
+ const Expr *OrigE = E->IgnoreParenCasts();
+ if (!OrigE)
+return StringRef();
As I remember, IgnoreParenCasts() can not return nullptr. So, we can turn this
into assertion.
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:262
+
+ if (const DeclRefExpr *DRE = dyn_cast_or_null(OrigE))
+return DRE->getFoundDecl()->getName();
OrigE is not nullptr s
[PATCH] D41409: [analyzer] Fix intermediate diagnostics on paths that go through operator new().
a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. Looks good! Repository: rC Clang https://reviews.llvm.org/D41409 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41408: [analyzer] NFC: Fix nothrow operator new definition in a test.
a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. Looks good, just a minor nit. Comment at: test/Analysis/NewDelete-custom.cpp:7 -#if !LEAKS +#if !(LEAKS && !ALLOCATOR_INLINING) // expected-no-diagnostics Double negation can be simplified a bit: `#if !LEAKS || ALLOCATOR_INLINING` Repository: rC Clang https://reviews.llvm.org/D41408 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41444: [ASTImporterTest] Make testing under '-fdelayed-template-parsing' mandatory
a.sidorin created this revision.
a.sidorin added reviewers: NoQ, xazax.hun, szepet.
Herald added subscribers: cfe-commits, rnkovacs.
While running ASTImporterTests, we often forget about Windows MSVC buildbots
which enable '-fdelayed-template-parsing' by default. It takes reviewing time
to find such issues as well as unexpected buildbot failures. To solve this
issue, I suggest making '-fdelayed-template-parsing' mandatory so this problem
can be caught during development.
Repository:
rC Clang
https://reviews.llvm.org/D41444
Files:
unittests/AST/ASTImporterTest.cpp
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -33,10 +33,10 @@
Args.insert(Args.end(), { "-x", "c", "-std=c89" });
break;
case Lang_CXX:
-Args.push_back("-std=c++98");
+Args.insert(Args.end(), {"-std=c++98", "-fdelayed-template-parsing"});
break;
case Lang_CXX11:
-Args.push_back("-std=c++11");
+Args.insert(Args.end(), {"-std=c++11", "-fdelayed-template-parsing"});
break;
case Lang_OpenCL:
case Lang_OBJCXX:
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -33,10 +33,10 @@
Args.insert(Args.end(), { "-x", "c", "-std=c89" });
break;
case Lang_CXX:
-Args.push_back("-std=c++98");
+Args.insert(Args.end(), {"-std=c++98", "-fdelayed-template-parsing"});
break;
case Lang_CXX11:
-Args.push_back("-std=c++11");
+Args.insert(Args.end(), {"-std=c++11", "-fdelayed-template-parsing"});
break;
case Lang_OpenCL:
case Lang_OBJCXX:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41444: [ASTImporterTest] Make testing under '-fdelayed-template-parsing' mandatory
a.sidorin added a comment. In https://reviews.llvm.org/D41444#960841, @xazax.hun wrote: > Is it possible that this will hide other problems? Wouldn't it be better to > run the tests twice once with this argument and once without it? I don't think so. In fact, without instantiation, we are not even able to check semantic code correctness inside templates. So, we are solving this problem as well. Repository: rC Clang https://reviews.llvm.org/D41444 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41444: [ASTImporterTest] Make testing under '-fdelayed-template-parsing' mandatory
a.sidorin added a comment. In case of `-fdelayed-template-parsing`, this code won't be even visible to the Importer. In my opinion, we shouldn't care about code we're not going to import. If we want to import it, we should make it visible and instantiate it. In this case it will not compile. However, I completely agree with the statement that testing of two options is better. The question is how to design unit testing for different command line options. I'll make a try and update the patch. Unfortunately, the new version is much bigger than the source patch. I'm not also sure that new design is scalable if we want to introduce more options in future. Any suggestions on this are welcome. Also, I still think we should rather not apply template-related patches until this testing is implemented. Gabor, Peter, do you agree? Repository: rC Clang https://reviews.llvm.org/D41444 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41444: [ASTImporterTest] Make testing under '-fdelayed-template-parsing' mandatory
a.sidorin updated this revision to Diff 127748.
a.sidorin added a comment.
Test both with and without '-fdelayed-template-parsing' in C++ mode.
Repository:
rC Clang
https://reviews.llvm.org/D41444
Files:
unittests/AST/ASTImporterTest.cpp
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -22,38 +22,50 @@
namespace clang {
namespace ast_matchers {
-typedef std::vector StringVector;
+typedef std::vector ArgVector;
+typedef std::vector RunOptions;
-void getLangArgs(Language Lang, StringVector &Args) {
+static bool isCXX(Language Lang) {
+ return Lang == Lang_CXX || Lang == Lang_CXX11;
+}
+
+static RunOptions getRunOptionsForLanguage(Language Lang) {
+ ArgVector BasicArgs;
+ // Test with basic arguments.
switch (Lang) {
case Lang_C:
-Args.insert(Args.end(), { "-x", "c", "-std=c99" });
+BasicArgs = {"-x", "c", "-std=c99"};
break;
case Lang_C89:
-Args.insert(Args.end(), { "-x", "c", "-std=c89" });
+BasicArgs = {"-x", "c", "-std=c89"};
break;
case Lang_CXX:
-Args.push_back("-std=c++98");
+BasicArgs = {"-std=c++98"};
break;
case Lang_CXX11:
-Args.push_back("-std=c++11");
+BasicArgs = {"-std=c++11"};
break;
case Lang_OpenCL:
case Lang_OBJCXX:
-break;
+llvm_unreachable("Not implemented yet!");
}
+
+ // For C++, test with "-fdelayed-template-parsing" enabled to handle MSVC
+ // default behaviour.
+ if (isCXX(Lang)) {
+ArgVector ArgsForDelayedTemplateParse = BasicArgs;
+ArgsForDelayedTemplateParse.emplace_back("-fdelayed-template-parsing");
+return {BasicArgs, ArgsForDelayedTemplateParse};
+ }
+
+ return {BasicArgs};
}
template
testing::AssertionResult
-testImport(const std::string &FromCode, Language FromLang,
- const std::string &ToCode, Language ToLang,
- MatchVerifier &Verifier,
- const MatcherType &AMatcher) {
- StringVector FromArgs, ToArgs;
- getLangArgs(FromLang, FromArgs);
- getLangArgs(ToLang, ToArgs);
-
+testImport(const ArgVector &FromArgs, const ArgVector &ToArgs,
+ const std::string &FromCode, const std::string &ToCode,
+ MatchVerifier &Verifier, const MatcherType &AMatcher) {
const char *const InputFileName = "input.cc";
const char *const OutputFileName = "output.cc";
@@ -92,7 +104,7 @@
return testing::AssertionFailure() << "Import failed, nullptr returned!";
// This should dump source locations and assert if some source locations
- // were not imported
+ // were not imported.
SmallString<1024> ImportChecker;
llvm::raw_svector_ostream ToNothing(ImportChecker);
ToCtx.getTranslationUnitDecl()->print(ToNothing);
@@ -104,148 +116,154 @@
return Verifier.match(Imported, AMatcher);
}
+template
+void testImport(const std::string &FromCode, Language FromLang,
+const std::string &ToCode, Language ToLang,
+MatchVerifier &Verifier,
+const MatcherType &AMatcher) {
+ auto RunOptsFrom = getRunOptionsForLanguage(FromLang);
+ auto RunOptsTo = getRunOptionsForLanguage(ToLang);
+ for (const auto &FromArgs : RunOptsFrom)
+for (const auto &ToArgs : RunOptsTo)
+ EXPECT_TRUE(testImport(FromArgs, ToArgs, FromCode, ToCode,
+ Verifier, AMatcher));
+}
+
+
TEST(ImportExpr, ImportStringLiteral) {
MatchVerifier Verifier;
- EXPECT_TRUE(testImport("void declToImport() { \"foo\"; }",
- Lang_CXX, "", Lang_CXX, Verifier,
- functionDecl(
- hasBody(
- compoundStmt(
- has(
- stringLiteral(
- hasType(
- asString("const char [4]");
- EXPECT_TRUE(testImport("void declToImport() { L\"foo\"; }",
- Lang_CXX, "", Lang_CXX, Verifier,
- functionDecl(
- hasBody(
- compoundStmt(
- has(
- stringLiteral(
- hasType(
- asString("const wchar_t [4]");
- EXPECT_TRUE(testImport("void declToImport() { \"foo\" \"bar\"; }",
- Lang_CXX, "", Lang_CXX, Verifier,
- functionDecl(
- hasBody(
- compoundStmt(
- has(
- stringLiteral(
- hasType(
- asString("const char [7]");
+ testImport("void declToImport() { \"foo\"; }",
+ Lang_CXX, "", Lang_CXX, Verifier,
+ functionDecl(
+ hasBody(
