[PATCH] D57906: [CTU] Do not allow different CPP dialects in CTU
This revision was automatically updated to reflect the committed changes.
martong marked an inline comment as done.
Closed by commit rC355096: [CTU] Do not allow different CPP dialects in CTU
(authored by martong, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D57906?vs=185926=188740#toc
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57906/new/
https://reviews.llvm.org/D57906
Files:
include/clang/CrossTU/CrossTranslationUnit.h
lib/CrossTU/CrossTranslationUnit.cpp
Index: lib/CrossTU/CrossTranslationUnit.cpp
===
--- lib/CrossTU/CrossTranslationUnit.cpp
+++ lib/CrossTU/CrossTranslationUnit.cpp
@@ -42,6 +42,7 @@
"requested function's body");
STATISTIC(NumTripleMismatch, "The # of triple mismatches");
STATISTIC(NumLangMismatch, "The # of language mismatches");
+STATISTIC(NumLangDialectMismatch, "The # of language dialect mismatches");
// Same as Triple's equality operator, but we check a field only if that is
// known in both instances.
@@ -99,6 +100,8 @@
return "Triple mismatch";
case index_error_code::lang_mismatch:
return "Language mismatch";
+case index_error_code::lang_dialect_mismatch:
+ return "Language dialect mismatch";
}
llvm_unreachable("Unrecognized index_error_code.");
}
@@ -228,6 +231,7 @@
const auto = Context.getLangOpts();
const auto = Unit->getASTContext().getLangOpts();
+
// FIXME: Currenty we do not support CTU across C++ and C and across
// different dialects of C++.
if (LangTo.CPlusPlus != LangFrom.CPlusPlus) {
@@ -235,6 +239,28 @@
return llvm::make_error(index_error_code::lang_mismatch);
}
+ // If CPP dialects are different then return with error.
+ //
+ // Consider this STL code:
+ // template
+ // struct __alloc_traits
+ // #if __cplusplus >= 201103L
+ // : std::allocator_traits<_Alloc>
+ // #endif
+ // { // ...
+ // };
+ // This class template would create ODR errors during merging the two units,
+ // since in one translation unit the class template has a base class, however
+ // in the other unit it has none.
+ if (LangTo.CPlusPlus11 != LangFrom.CPlusPlus11 ||
+ LangTo.CPlusPlus14 != LangFrom.CPlusPlus14 ||
+ LangTo.CPlusPlus17 != LangFrom.CPlusPlus17 ||
+ LangTo.CPlusPlus2a != LangFrom.CPlusPlus2a) {
+++NumLangDialectMismatch;
+return llvm::make_error(
+index_error_code::lang_dialect_mismatch);
+ }
+
TranslationUnitDecl *TU = Unit->getASTContext().getTranslationUnitDecl();
if (const FunctionDecl *ResultDecl =
findFunctionInDeclContext(TU, LookupFnName))
Index: include/clang/CrossTU/CrossTranslationUnit.h
===
--- include/clang/CrossTU/CrossTranslationUnit.h
+++ include/clang/CrossTU/CrossTranslationUnit.h
@@ -43,7 +43,8 @@
failed_to_get_external_ast,
failed_to_generate_usr,
triple_mismatch,
- lang_mismatch
+ lang_mismatch,
+ lang_dialect_mismatch
};
class IndexError : public llvm::ErrorInfo {
Index: lib/CrossTU/CrossTranslationUnit.cpp
===
--- lib/CrossTU/CrossTranslationUnit.cpp
+++ lib/CrossTU/CrossTranslationUnit.cpp
@@ -42,6 +42,7 @@
"requested function's body");
STATISTIC(NumTripleMismatch, "The # of triple mismatches");
STATISTIC(NumLangMismatch, "The # of language mismatches");
+STATISTIC(NumLangDialectMismatch, "The # of language dialect mismatches");
// Same as Triple's equality operator, but we check a field only if that is
// known in both instances.
@@ -99,6 +100,8 @@
return "Triple mismatch";
case index_error_code::lang_mismatch:
return "Language mismatch";
+case index_error_code::lang_dialect_mismatch:
+ return "Language dialect mismatch";
}
llvm_unreachable("Unrecognized index_error_code.");
}
@@ -228,6 +231,7 @@
const auto = Context.getLangOpts();
const auto = Unit->getASTContext().getLangOpts();
+
// FIXME: Currenty we do not support CTU across C++ and C and across
// different dialects of C++.
if (LangTo.CPlusPlus != LangFrom.CPlusPlus) {
@@ -235,6 +239,28 @@
return llvm::make_error(index_error_code::lang_mismatch);
}
+ // If CPP dialects are different then return with error.
+ //
+ // Consider this STL code:
+ // template
+ // struct __alloc_traits
+ // #if __cplusplus >= 201103L
+ // : std::allocator_traits<_Alloc>
+ // #endif
+ // { // ...
+ // };
+ // This class template would create ODR errors during merging the two units,
+ // since in one translation unit the class template has a base class, however
+ // in the other unit it has none.
+ if (LangTo.CPlusPlus11 != LangFrom.CPlusPlus11 ||
+ LangTo.CPlusPlus14 != LangFrom.CPlusPlus14 ||
+ LangTo.CPlusPlus17 != LangFrom.CPlusPlus17 ||
+
[PATCH] D57906: [CTU] Do not allow different CPP dialects in CTU
xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM! I think we should commit this as is for now but maybe adding a TODO comment to summarize the problem would be nice. Maybe we could have an isSameDialect or similar method within LangOpts, so it is harder to break this code. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57906/new/ https://reviews.llvm.org/D57906 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57906: [CTU] Do not allow different CPP dialects in CTU
martong marked 2 inline comments as done. martong added inline comments. Comment at: lib/CrossTU/CrossTranslationUnit.cpp:255 + // in the other unit it has none. + if (LangTo.CPlusPlus11 != LangFrom.CPlusPlus11 || + LangTo.CPlusPlus14 != LangFrom.CPlusPlus14 || r.stahl wrote: > This is likely to become a bug in the future, but I didn't see a better way > to compare dialects. > > Is there a way to add a test that lights up once there is a new dialect? > > Would it be too pessimistic to compare the entire LangOpts? Some stuff in > there should even still produce errors, right? For example "GnuMode". I > skimmed over them and didn't find an obvious one that would differ between > translation units of the same project. Maybe the template depth could be set > selectively, but to fix that we could mask "COMPATIBLE_" and "BENIGN_" opts. > This is likely to become a bug in the future, but I didn't see a better way > to compare dialects. > Is there a way to add a test that lights up once there is a new dialect? `LANGOPTS` are defined as bitfields: `#define LANGOPT(Name, Bits, Default, Description) unsigned Name : Bits;` I can't think of any solution to avoid the future bug, because there is no way to enumerate all the C++ dialects. I am going to ask around about this on cfe-dev, maybe somebody will have an idea. > Would it be too pessimistic to compare the entire LangOpts? I think that would be too pessimistic. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57906/new/ https://reviews.llvm.org/D57906 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57906: [CTU] Do not allow different CPP dialects in CTU
martong added a comment. > How is #if __cplusplus >= 201103L qualitatively different from #ifndef NDEBUG > or #if MYLIB_ABI_VERSION==2 or #if __DATE__ == "2018-04-01"? Ideally, all macros should be the same in the two TUs... If we were very strict then we could check for that, but that might be just too strict. If there is an ODR error (either because of a macro mismatch or for other reasons) then structural equivalency will diagnose that, but may be nontrivial for the user to identify what causes the ODR error. Checking the language and the language dialect is easy and it prevents such disturbing errors early. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57906/new/ https://reviews.llvm.org/D57906 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57906: [CTU] Do not allow different CPP dialects in CTU
r.stahl added inline comments. Comment at: lib/CrossTU/CrossTranslationUnit.cpp:255 + // in the other unit it has none. + if (LangTo.CPlusPlus11 != LangFrom.CPlusPlus11 || + LangTo.CPlusPlus14 != LangFrom.CPlusPlus14 || This is likely to become a bug in the future, but I didn't see a better way to compare dialects. Is there a way to add a test that lights up once there is a new dialect? Would it be too pessimistic to compare the entire LangOpts? Some stuff in there should even still produce errors, right? For example "GnuMode". I skimmed over them and didn't find an obvious one that would differ between translation units of the same project. Maybe the template depth could be set selectively, but to fix that we could mask "COMPATIBLE_" and "BENIGN_" opts. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57906/new/ https://reviews.llvm.org/D57906 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57906: [CTU] Do not allow different CPP dialects in CTU
Quuxplusone added a comment.
> Consider this STL code:
>
> template
> struct __alloc_traits
> #if __cplusplus >= 201103L
> : std::allocator_traits<_Alloc>
> #endif
> { // ...
> };
>
>
> This class template would create ODR errors during merging the two units,
> since in one translation unit the class template has a base class, however
> in the other unit it has none.
How is `#if __cplusplus >= 201103L` qualitatively different from `#ifndef
NDEBUG` or `#if MYLIB_ABI_VERSION==2` or `#if __DATE__ == "2018-04-01"`?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57906/new/
https://reviews.llvm.org/D57906
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57906: [CTU] Do not allow different CPP dialects in CTU
martong added inline comments.
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:232
+
+ // We do not support CTU across languages (C vs C++).
if (LangTo.CPlusPlus != LangFrom.CPlusPlus) {
a_sidorin wrote:
> The comment change looks strange.
Ok, I reverted this hunk.
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:255
+ LangTo.CPlusPlus2a != LangFrom.CPlusPlus2a) {
+++NumLangMismatch;
+return llvm::make_error(index_error_code::lang_mismatch);
a_sidorin wrote:
> Should we create another counter, like NumLangDialectMismatch?
Okay, I added the new error code `lang_dialect_mismatch` too.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57906/new/
https://reviews.llvm.org/D57906
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57906: [CTU] Do not allow different CPP dialects in CTU
martong updated this revision to Diff 185926.
martong marked 4 inline comments as done.
martong added a comment.
- Revert comment change
- Add lang_dialect_mismatch and stats for that
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57906/new/
https://reviews.llvm.org/D57906
Files:
include/clang/CrossTU/CrossTranslationUnit.h
lib/CrossTU/CrossTranslationUnit.cpp
Index: lib/CrossTU/CrossTranslationUnit.cpp
===
--- lib/CrossTU/CrossTranslationUnit.cpp
+++ lib/CrossTU/CrossTranslationUnit.cpp
@@ -42,6 +42,7 @@
"requested function's body");
STATISTIC(NumTripleMismatch, "The # of triple mismatches");
STATISTIC(NumLangMismatch, "The # of language mismatches");
+STATISTIC(NumLangDialectMismatch, "The # of language dialect mismatches");
// Same as Triple's equality operator, but we check a field only if that is
// known in both instances.
@@ -99,6 +100,8 @@
return "Triple mismatch";
case index_error_code::lang_mismatch:
return "Language mismatch";
+case index_error_code::lang_dialect_mismatch:
+ return "Language dialect mismatch";
}
llvm_unreachable("Unrecognized index_error_code.");
}
@@ -228,6 +231,7 @@
const auto = Context.getLangOpts();
const auto = Unit->getASTContext().getLangOpts();
+
// FIXME: Currenty we do not support CTU across C++ and C and across
// different dialects of C++.
if (LangTo.CPlusPlus != LangFrom.CPlusPlus) {
@@ -235,6 +239,28 @@
return llvm::make_error(index_error_code::lang_mismatch);
}
+ // If CPP dialects are different then return with error.
+ //
+ // Consider this STL code:
+ // template
+ // struct __alloc_traits
+ // #if __cplusplus >= 201103L
+ // : std::allocator_traits<_Alloc>
+ // #endif
+ // { // ...
+ // };
+ // This class template would create ODR errors during merging the two units,
+ // since in one translation unit the class template has a base class, however
+ // in the other unit it has none.
+ if (LangTo.CPlusPlus11 != LangFrom.CPlusPlus11 ||
+ LangTo.CPlusPlus14 != LangFrom.CPlusPlus14 ||
+ LangTo.CPlusPlus17 != LangFrom.CPlusPlus17 ||
+ LangTo.CPlusPlus2a != LangFrom.CPlusPlus2a) {
+++NumLangDialectMismatch;
+return llvm::make_error(
+index_error_code::lang_dialect_mismatch);
+ }
+
TranslationUnitDecl *TU = Unit->getASTContext().getTranslationUnitDecl();
if (const FunctionDecl *ResultDecl =
findFunctionInDeclContext(TU, LookupFnName))
Index: include/clang/CrossTU/CrossTranslationUnit.h
===
--- include/clang/CrossTU/CrossTranslationUnit.h
+++ include/clang/CrossTU/CrossTranslationUnit.h
@@ -43,7 +43,8 @@
failed_to_get_external_ast,
failed_to_generate_usr,
triple_mismatch,
- lang_mismatch
+ lang_mismatch,
+ lang_dialect_mismatch
};
class IndexError : public llvm::ErrorInfo {
Index: lib/CrossTU/CrossTranslationUnit.cpp
===
--- lib/CrossTU/CrossTranslationUnit.cpp
+++ lib/CrossTU/CrossTranslationUnit.cpp
@@ -42,6 +42,7 @@
"requested function's body");
STATISTIC(NumTripleMismatch, "The # of triple mismatches");
STATISTIC(NumLangMismatch, "The # of language mismatches");
+STATISTIC(NumLangDialectMismatch, "The # of language dialect mismatches");
// Same as Triple's equality operator, but we check a field only if that is
// known in both instances.
@@ -99,6 +100,8 @@
return "Triple mismatch";
case index_error_code::lang_mismatch:
return "Language mismatch";
+case index_error_code::lang_dialect_mismatch:
+ return "Language dialect mismatch";
}
llvm_unreachable("Unrecognized index_error_code.");
}
@@ -228,6 +231,7 @@
const auto = Context.getLangOpts();
const auto = Unit->getASTContext().getLangOpts();
+
// FIXME: Currenty we do not support CTU across C++ and C and across
// different dialects of C++.
if (LangTo.CPlusPlus != LangFrom.CPlusPlus) {
@@ -235,6 +239,28 @@
return llvm::make_error(index_error_code::lang_mismatch);
}
+ // If CPP dialects are different then return with error.
+ //
+ // Consider this STL code:
+ // template
+ // struct __alloc_traits
+ // #if __cplusplus >= 201103L
+ // : std::allocator_traits<_Alloc>
+ // #endif
+ // { // ...
+ // };
+ // This class template would create ODR errors during merging the two units,
+ // since in one translation unit the class template has a base class, however
+ // in the other unit it has none.
+ if (LangTo.CPlusPlus11 != LangFrom.CPlusPlus11 ||
+ LangTo.CPlusPlus14 != LangFrom.CPlusPlus14 ||
+ LangTo.CPlusPlus17 != LangFrom.CPlusPlus17 ||
+ LangTo.CPlusPlus2a != LangFrom.CPlusPlus2a) {
+++NumLangDialectMismatch;
+return llvm::make_error(
+
[PATCH] D57906: [CTU] Do not allow different CPP dialects in CTU
a_sidorin added a comment.
Hi Gabor,
Please find my comments inline.
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:232
+
+ // We do not support CTU across languages (C vs C++).
if (LangTo.CPlusPlus != LangFrom.CPlusPlus) {
The comment change looks strange.
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:255
+ LangTo.CPlusPlus2a != LangFrom.CPlusPlus2a) {
+++NumLangMismatch;
+return llvm::make_error(index_error_code::lang_mismatch);
Should we create another counter, like NumLangDialectMismatch?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57906/new/
https://reviews.llvm.org/D57906
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57906: [CTU] Do not allow different CPP dialects in CTU
martong created this revision.
martong added reviewers: xazax.hun, a_sidorin, r.stahl.
Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs.
Herald added a project: clang.
If CPP dialects are different then return with error.
Consider this STL code:
template
struct __alloc_traits
#if __cplusplus >= 201103L
: std::allocator_traits<_Alloc>
#endif
{ // ...
};
This class template would create ODR errors during merging the two units,
since in one translation unit the class template has a base class, however
in the other unit it has none.
Repository:
rC Clang
https://reviews.llvm.org/D57906
Files:
lib/CrossTU/CrossTranslationUnit.cpp
Index: lib/CrossTU/CrossTranslationUnit.cpp
===
--- lib/CrossTU/CrossTranslationUnit.cpp
+++ lib/CrossTU/CrossTranslationUnit.cpp
@@ -228,13 +228,34 @@
const auto = Context.getLangOpts();
const auto = Unit->getASTContext().getLangOpts();
- // FIXME: Currenty we do not support CTU across C++ and C and across
- // different dialects of C++.
+
+ // We do not support CTU across languages (C vs C++).
if (LangTo.CPlusPlus != LangFrom.CPlusPlus) {
++NumLangMismatch;
return llvm::make_error(index_error_code::lang_mismatch);
}
+ // If CPP dialects are different then return with error.
+ //
+ // Consider this STL code:
+ // template
+ // struct __alloc_traits
+ // #if __cplusplus >= 201103L
+ // : std::allocator_traits<_Alloc>
+ // #endif
+ // { // ...
+ // };
+ // This class template would create ODR errors during merging the two units,
+ // since in one translation unit the class template has a base class, however
+ // in the other unit it has none.
+ if (LangTo.CPlusPlus11 != LangFrom.CPlusPlus11 ||
+ LangTo.CPlusPlus14 != LangFrom.CPlusPlus14 ||
+ LangTo.CPlusPlus17 != LangFrom.CPlusPlus17 ||
+ LangTo.CPlusPlus2a != LangFrom.CPlusPlus2a) {
+++NumLangMismatch;
+return llvm::make_error(index_error_code::lang_mismatch);
+ }
+
TranslationUnitDecl *TU = Unit->getASTContext().getTranslationUnitDecl();
if (const FunctionDecl *ResultDecl =
findFunctionInDeclContext(TU, LookupFnName))
Index: lib/CrossTU/CrossTranslationUnit.cpp
===
--- lib/CrossTU/CrossTranslationUnit.cpp
+++ lib/CrossTU/CrossTranslationUnit.cpp
@@ -228,13 +228,34 @@
const auto = Context.getLangOpts();
const auto = Unit->getASTContext().getLangOpts();
- // FIXME: Currenty we do not support CTU across C++ and C and across
- // different dialects of C++.
+
+ // We do not support CTU across languages (C vs C++).
if (LangTo.CPlusPlus != LangFrom.CPlusPlus) {
++NumLangMismatch;
return llvm::make_error(index_error_code::lang_mismatch);
}
+ // If CPP dialects are different then return with error.
+ //
+ // Consider this STL code:
+ // template
+ // struct __alloc_traits
+ // #if __cplusplus >= 201103L
+ // : std::allocator_traits<_Alloc>
+ // #endif
+ // { // ...
+ // };
+ // This class template would create ODR errors during merging the two units,
+ // since in one translation unit the class template has a base class, however
+ // in the other unit it has none.
+ if (LangTo.CPlusPlus11 != LangFrom.CPlusPlus11 ||
+ LangTo.CPlusPlus14 != LangFrom.CPlusPlus14 ||
+ LangTo.CPlusPlus17 != LangFrom.CPlusPlus17 ||
+ LangTo.CPlusPlus2a != LangFrom.CPlusPlus2a) {
+++NumLangMismatch;
+return llvm::make_error(index_error_code::lang_mismatch);
+ }
+
TranslationUnitDecl *TU = Unit->getASTContext().getTranslationUnitDecl();
if (const FunctionDecl *ResultDecl =
findFunctionInDeclContext(TU, LookupFnName))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
