Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang] Improve source location in binary type traits diagnostics #88097

Merged
merged 2 commits into from
Apr 10, 2024

Conversation

Endilll
Copy link
Contributor

@Endilll Endilll commented Apr 9, 2024

This patch takes advantage of a recent NFC change that refactored EvaluateBinaryTypeTrait() to accept TypeSourceInfo instead of QualType c7db450.
Before:

test2.cpp:105:55: error: variable length arrays are not supported in '__is_layout_compatible'
  105 |   static_assert(!__is_layout_compatible(int[n], int[n]));
      |                                                       ^
test2.cpp:125:76: error: incomplete type 'CStructIncomplete' where a complete type is required
  125 |   static_assert(__is_layout_compatible(CStructIncomplete, CStructIncomplete));
      |                                                                            ^

After:

test2.cpp:105:41: error: variable length arrays are not supported in '__is_layout_compatible'
  105 |   static_assert(!__is_layout_compatible(int[n], int[n]));
      |                                         ^
test2.cpp:125:40: error: incomplete type 'CStructIncomplete' where a complete type is required
  125 |   static_assert(__is_layout_compatible(CStructIncomplete, CStructIncomplete));
      |                                        ^

@Endilll Endilll added c++ clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer labels Apr 9, 2024
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 9, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 9, 2024

@llvm/pr-subscribers-clang

Author: Vlad Serebrennikov (Endilll)

Changes

This patch takes advantage of a recent NFC change that refactored EvaluateBinaryTypeTrait() to accept TypeSourceInfo instead of QualType c7db450.
Before:

test2.cpp:105:55: error: variable length arrays are not supported in '__is_layout_compatible'
  105 |   static_assert(!__is_layout_compatible(int[n], int[n]));
      |                                                       ^
test2.cpp:125:76: error: incomplete type 'CStructIncomplete' where a complete type is required
  125 |   static_assert(__is_layout_compatible(CStructIncomplete, CStructIncomplete));
      |                                                                            ^

After:

test2.cpp:105:41: error: variable length arrays are not supported in '__is_layout_compatible'
  105 |   static_assert(!__is_layout_compatible(int[n], int[n]));
      |                                         ^
test2.cpp:125:40: error: incomplete type 'CStructIncomplete' where a complete type is required
  125 |   static_assert(__is_layout_compatible(CStructIncomplete, CStructIncomplete));
      |                                        ^

Full diff: https://github.com/llvm/llvm-project/pull/88097.diff

2 Files Affected:

  • (modified) clang/lib/Sema/SemaExprCXX.cpp (+12-9)
  • (modified) clang/test/SemaCXX/type-traits.cpp (+1)
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index dee6b658cd0054..7d7be27a862c2a 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -5843,7 +5843,7 @@ static bool EvaluateBinaryTypeTrait(Sema &Self, TypeTrait BTT, const TypeSourceI
         return false;
 
       if (Self.RequireCompleteType(
-              KeyLoc, RhsT, diag::err_incomplete_type_used_in_type_trait_expr))
+              Rhs->getTypeLoc().getBeginLoc(), RhsT, diag::err_incomplete_type_used_in_type_trait_expr))
         return false;
 
       return BaseInterface->isSuperClassOf(DerivedInterface);
@@ -5866,7 +5866,7 @@ static bool EvaluateBinaryTypeTrait(Sema &Self, TypeTrait BTT, const TypeSourceI
     //   If Base and Derived are class types and are different types
     //   (ignoring possible cv-qualifiers) then Derived shall be a
     //   complete type.
-    if (Self.RequireCompleteType(KeyLoc, RhsT,
+    if (Self.RequireCompleteType(Rhs->getTypeLoc().getBeginLoc(), RhsT,
                           diag::err_incomplete_type_used_in_type_trait_expr))
       return false;
 
@@ -5919,7 +5919,7 @@ static bool EvaluateBinaryTypeTrait(Sema &Self, TypeTrait BTT, const TypeSourceI
       return LhsT->isVoidType();
 
     // A function definition requires a complete, non-abstract return type.
-    if (!Self.isCompleteType(KeyLoc, RhsT) || Self.isAbstractType(KeyLoc, RhsT))
+    if (!Self.isCompleteType(Rhs->getTypeLoc().getBeginLoc(), RhsT) || Self.isAbstractType(Rhs->getTypeLoc().getBeginLoc(), RhsT))
       return false;
 
     // Compute the result of add_rvalue_reference.
@@ -5969,11 +5969,11 @@ static bool EvaluateBinaryTypeTrait(Sema &Self, TypeTrait BTT, const TypeSourceI
     //   For both, T and U shall be complete types, (possibly cv-qualified)
     //   void, or arrays of unknown bound.
     if (!LhsT->isVoidType() && !LhsT->isIncompleteArrayType() &&
-        Self.RequireCompleteType(KeyLoc, LhsT,
+        Self.RequireCompleteType(Lhs->getTypeLoc().getBeginLoc(), LhsT,
           diag::err_incomplete_type_used_in_type_trait_expr))
       return false;
     if (!RhsT->isVoidType() && !RhsT->isIncompleteArrayType() &&
-        Self.RequireCompleteType(KeyLoc, RhsT,
+        Self.RequireCompleteType(Rhs->getTypeLoc().getBeginLoc(), RhsT,
           diag::err_incomplete_type_used_in_type_trait_expr))
       return false;
 
@@ -6029,12 +6029,15 @@ static bool EvaluateBinaryTypeTrait(Sema &Self, TypeTrait BTT, const TypeSourceI
   }
   case BTT_IsLayoutCompatible: {
     if (!LhsT->isVoidType() && !LhsT->isIncompleteArrayType())
-      Self.RequireCompleteType(KeyLoc, LhsT, diag::err_incomplete_type);
+      Self.RequireCompleteType(Lhs->getTypeLoc().getBeginLoc(), LhsT, diag::err_incomplete_type);
     if (!RhsT->isVoidType() && !RhsT->isIncompleteArrayType())
-      Self.RequireCompleteType(KeyLoc, RhsT, diag::err_incomplete_type);
+      Self.RequireCompleteType(Rhs->getTypeLoc().getBeginLoc(), RhsT, diag::err_incomplete_type);
 
-    if (LhsT->isVariableArrayType() || RhsT->isVariableArrayType())
-      Self.Diag(KeyLoc, diag::err_vla_unsupported)
+    if (LhsT->isVariableArrayType())
+      Self.Diag(Lhs->getTypeLoc().getBeginLoc(), diag::err_vla_unsupported)
+          << 1 << tok::kw___is_layout_compatible;
+    if (RhsT->isVariableArrayType())
+      Self.Diag(Rhs->getTypeLoc().getBeginLoc(), diag::err_vla_unsupported)
           << 1 << tok::kw___is_layout_compatible;
     return Self.IsLayoutCompatible(LhsT, RhsT);
   }
diff --git a/clang/test/SemaCXX/type-traits.cpp b/clang/test/SemaCXX/type-traits.cpp
index e29763714341e7..421d3007d27ffe 100644
--- a/clang/test/SemaCXX/type-traits.cpp
+++ b/clang/test/SemaCXX/type-traits.cpp
@@ -1759,6 +1759,7 @@ void is_layout_compatible(int n)
   // expected-error@-1 {{variable length arrays are not supported in '__is_layout_compatible'}}
   static_assert(!__is_layout_compatible(int[n], int[n]));
   // expected-error@-1 {{variable length arrays are not supported in '__is_layout_compatible'}}
+  // expected-error@-2 {{variable length arrays are not supported in '__is_layout_compatible'}}
   static_assert(__is_layout_compatible(int&, int&));
   static_assert(!__is_layout_compatible(int&, char&));
   static_assert(__is_layout_compatible(void(int), void(int)));

Copy link

github-actions bot commented Apr 9, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@tbaederr
Copy link
Contributor

tbaederr commented Apr 9, 2024

Do we now have enough information to pass a source range as well?

@Endilll
Copy link
Contributor Author

Endilll commented Apr 9, 2024

Do we now have enough information to pass a source range as well?

Yes, TypeLoc inside TypeSourceInfo has two SourceLocation objects that represent the range.
In order to limit the scope of the PR, I'm not refactoring RequireCompleteType and friends to accept TypeSourceInfo, but that's an obvious next step. Diagnostics would benefit from source range.

@Endilll Endilll merged commit 817c832 into llvm:main Apr 10, 2024
4 checks passed
@Endilll Endilll deleted the btt-source-loc branch April 10, 2024 04:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants