[PATCH] D123009: [Sema] Enum conversion warning when one signed and other unsigned.

[Micah Weston via Phabricator via cfe-commits]

red1bluelost created this revision.
red1bluelost added reviewers: rsmith, lebedev.ri, aaron.ballman.
Herald added a project: All.
red1bluelost requested review of this revision.
Herald added a project: clang.

Ensures an -Wenum-conversion warning happens when one of the enums is
signed and the other is unsigned. Also adds a test file to verify these
warnings.

This warning would not happen since the -Wsign-conversion would make a
diagnostic then return, never allowing the -Wenum-conversion checks.

For example:

  C
  enum PE { P = -1 };
  enum NE { N };
  enum NE conv(enum PE E) { return E; }

Before this would only create a diagnostic with -Wsign-conversion and never on 
-Wenum-conversion. Now it will create a diagnostic for both -Wsign-conversion 
and -Wenum-conversion.

I could change it to just warn on -Wenum-conversion as that was what I initially
did. Seeing PR35200 (or GitHub Issue 316268), I let both diagnostics check so 
that
the sign conversion could generate a warning.

This was tested with `check-clang` and all tests passed along with the new test
file.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D123009

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/enum-enum-conversion.c


Index: clang/test/Sema/enum-enum-conversion.c
===
--- /dev/null
+++ clang/test/Sema/enum-enum-conversion.c
@@ -0,0 +1,39 @@
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -verify 
-Wenum-conversion -DENUM_CONV %s
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -verify 
-Wconversion %s
+
+enum NE1 { N1 = -1 };
+enum NE2 { N2 = -2 };
+enum PE1 { P1 };
+enum PE2 { P2 };
+
+enum PE2 f1(enum PE1 E) {
+#ifdef ENUM_CONV
+  return E; // expected-warning {{implicit conversion from enumeration type 
'enum PE1' to different enumeration type 'enum PE2'}}
+#else
+  return E; // expected-warning {{implicit conversion from enumeration type 
'enum PE1' to different enumeration type 'enum PE2'}}
+#endif
+}
+
+enum NE1 f2(enum PE1 E) {
+#ifdef ENUM_CONV
+  return E; // expected-warning {{implicit conversion from enumeration type 
'enum PE1' to different enumeration type 'enum NE1'}}
+#else
+  return E; // expected-warning {{implicit conversion from enumeration type 
'enum PE1' to different enumeration type 'enum NE1'}} expected-warning 
{{implicit conversion changes signedness: 'enum PE1' to 'enum NE1'}}
+#endif
+}
+
+enum PE1 f3(enum NE1 E) {
+#ifdef ENUM_CONV
+  return E; // expected-warning {{implicit conversion from enumeration type 
'enum NE1' to different enumeration type 'enum PE1'}}
+#else
+  return E; // expected-warning {{implicit conversion from enumeration type 
'enum NE1' to different enumeration type 'enum PE1'}} expected-warning 
{{implicit conversion changes signedness: 'enum NE1' to 'enum PE1'}}
+#endif
+}
+
+enum NE2 f4(enum NE1 E) {
+#ifdef ENUM_CONV
+  return E; // expected-warning {{implicit conversion from enumeration type 
'enum NE1' to different enumeration type 'enum NE2'}}
+#else
+  return E; // expected-warning {{implicit conversion from enumeration type 
'enum NE1' to different enumeration type 'enum NE2'}}
+#endif
+}
\ No newline at end of file
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13552,7 +13552,9 @@
   *ICContext = true;
 }
 
-return DiagnoseImpCast(S, E, T, CC, DiagID);
+DiagnoseImpCast(S, E, T, CC, DiagID);
+if (!isa(Target) || !isa(Source))
+  return;
   }
 
   // Diagnose conversions between different enumeration types.


Index: clang/test/Sema/enum-enum-conversion.c
===
--- /dev/null
+++ clang/test/Sema/enum-enum-conversion.c
@@ -0,0 +1,39 @@
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -verify -Wenum-conversion -DENUM_CONV %s
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -verify -Wconversion %s
+
+enum NE1 { N1 = -1 };
+enum NE2 { N2 = -2 };
+enum PE1 { P1 };
+enum PE2 { P2 };
+
+enum PE2 f1(enum PE1 E) {
+#ifdef ENUM_CONV
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum PE1' to different enumeration type 'enum PE2'}}
+#else
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum PE1' to different enumeration type 'enum PE2'}}
+#endif
+}
+
+enum NE1 f2(enum PE1 E) {
+#ifdef ENUM_CONV
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum PE1' to different enumeration type 'enum NE1'}}
+#else
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum PE1' to different enumeration type 'enum NE1'}} expected-warning {{implicit conversion changes signedness: 'enum PE1' to 'enum NE1'}}
+#endif
+}
+
+enum PE1 f3(enum NE1 E) {
+#ifdef ENUM_CONV
+  return E; // expected-warning {{implicit conversion from enumeratio

[PATCH] D123009: [Sema] Enum conversion warning when one signed and other unsigned.

[Micah Weston via Phabricator via cfe-commits]

red1bluelost updated this revision to Diff 424053.
red1bluelost added a comment.

Updates patch based on comments.

Silences the sign conversions on enum-to-enum so that only enum-to-enum will 
warn.
Simplifies the test case since now only one warning occurs.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123009/new/

https://reviews.llvm.org/D123009

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/enum-enum-conversion.c


Index: clang/test/Sema/enum-enum-conversion.c
===
--- /dev/null
+++ clang/test/Sema/enum-enum-conversion.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wenum-conversion 
-verify %s
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wconversion 
-verify %s
+
+// Signed enums
+enum SE1 { N1 = -1 };
+enum SE2 { N2 = -2 };
+// Unsigned unums
+enum UE1 { P1 };
+enum UE2 { P2 };
+
+enum UE2 f1(enum UE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 
'enum UE1' to different enumeration type 'enum UE2'}}
+}
+
+enum SE1 f2(enum UE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 
'enum UE1' to different enumeration type 'enum SE1'}}
+}
+
+enum UE1 f3(enum SE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 
'enum SE1' to different enumeration type 'enum UE1'}}
+}
+
+enum SE2 f4(enum SE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 
'enum SE1' to different enumeration type 'enum SE2'}}
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13534,9 +13534,10 @@
 // Fall through for non-constants to give a sign conversion warning.
   }
 
-  if ((TargetRange.NonNegative && !LikelySourceRange.NonNegative) ||
-  (!TargetRange.NonNegative && LikelySourceRange.NonNegative &&
-   LikelySourceRange.Width == TargetRange.Width)) {
+  if ((!isa(Target) || !isa(Source)) &&
+  ((TargetRange.NonNegative && !LikelySourceRange.NonNegative) ||
+   (!TargetRange.NonNegative && LikelySourceRange.NonNegative &&
+LikelySourceRange.Width == TargetRange.Width))) {
 if (S.SourceMgr.isInSystemMacro(CC))
   return;
 


Index: clang/test/Sema/enum-enum-conversion.c
===
--- /dev/null
+++ clang/test/Sema/enum-enum-conversion.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wenum-conversion -verify %s
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wconversion -verify %s
+
+// Signed enums
+enum SE1 { N1 = -1 };
+enum SE2 { N2 = -2 };
+// Unsigned unums
+enum UE1 { P1 };
+enum UE2 { P2 };
+
+enum UE2 f1(enum UE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum UE1' to different enumeration type 'enum UE2'}}
+}
+
+enum SE1 f2(enum UE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum UE1' to different enumeration type 'enum SE1'}}
+}
+
+enum UE1 f3(enum SE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum SE1' to different enumeration type 'enum UE1'}}
+}
+
+enum SE2 f4(enum SE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum SE1' to different enumeration type 'enum SE2'}}
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13534,9 +13534,10 @@
 // Fall through for non-constants to give a sign conversion warning.
   }
 
-  if ((TargetRange.NonNegative && !LikelySourceRange.NonNegative) ||
-  (!TargetRange.NonNegative && LikelySourceRange.NonNegative &&
-   LikelySourceRange.Width == TargetRange.Width)) {
+  if ((!isa(Target) || !isa(Source)) &&
+  ((TargetRange.NonNegative && !LikelySourceRange.NonNegative) ||
+   (!TargetRange.NonNegative && LikelySourceRange.NonNegative &&
+LikelySourceRange.Width == TargetRange.Width))) {
 if (S.SourceMgr.isInSystemMacro(CC))
   return;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123009: [Sema] Enum conversion warning when one signed and other unsigned.

[Micah Weston via Phabricator via cfe-commits]

red1bluelost marked 3 inline comments as done.
red1bluelost added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:13555-13557
+DiagnoseImpCast(S, E, T, CC, DiagID);
+if (!isa(Target) || !isa(Source))
+  return;

aaron.ballman wrote:
> I don't think this change is correct -- we *want* to silence the conversion 
> warnings in this case. GCC behaves that way as well: 
> https://godbolt.org/z/oona5Mr5T
Updated the code to silence the sign conversion and only warn the 
enum-conversion.



Comment at: clang/test/Sema/enum-enum-conversion.c:10-14
+#ifdef ENUM_CONV
+  return E; // expected-warning {{implicit conversion from enumeration type 
'enum PE1' to different enumeration type 'enum PE2'}}
+#else
+  return E; // expected-warning {{implicit conversion from enumeration type 
'enum PE1' to different enumeration type 'enum PE2'}}
+#endif

aaron.ballman wrote:
> Testing tip: instead of using the preprocessor and duplicating so much, you 
> can use a feature of `-verify` where you give it a prefix. e.g.,
> ```
> // RUN: ... -verify=stuff ...
> // RUN: ... -verify=other ...
> 
> void func(void) {
>   thing();  // stuff-warning {{"this is a warning about stuff}} \
>other-error {{"this is an error, it's different than stuff}}
> }
> ```
> Using this sort of style makes the tests much easier to read.
Thank! That is helpful to know!
I ended up not needing it here since the silencing of sign-conversion means 
only one warning will occur.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123009/new/

https://reviews.llvm.org/D123009

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123009: [Sema] Enum conversion warning when one signed and other unsigned.

[Micah Weston via Phabricator via cfe-commits]

red1bluelost updated this revision to Diff 426786.
red1bluelost added a comment.

Adds regression tests for enum to int sign conversion warnings and notes the
changes in the release notes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123009/new/

https://reviews.llvm.org/D123009

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/enum-enum-conversion.c
  clang/test/Sema/enum-sign-conversion.c

Index: clang/test/Sema/enum-sign-conversion.c
===
--- clang/test/Sema/enum-sign-conversion.c
+++ clang/test/Sema/enum-sign-conversion.c
@@ -1,13 +1,51 @@
-// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -verify -DUNSIGNED -Wsign-conversion %s
-// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only -verify -Wsign-conversion %s
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wsign-conversion -verify=unsigned,both %s
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wconversion -verify=unsigned,both %s
+// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only -verify -Wsign-conversion -verify=signed,both %s
 
 // PR35200
-enum X { A,B,C};
+enum X { A,
+ B,
+ C };
 int f(enum X x) {
-#ifdef UNSIGNED
-  return x; // expected-warning {{implicit conversion changes signedness: 'enum X' to 'int'}}
-#else
-  // expected-no-diagnostics
-  return x;
-#endif
+  return x; // unsigned-warning {{implicit conversion changes signedness: 'enum X' to 'int'}}
+}
+
+// Signed enums
+enum SE1 { N1 = -1 };
+enum SE2 { N2 = -2 };
+// Unsigned unums
+enum UE1 { P1 };
+enum UE2 { P2 };
+
+// ensure no regression with enum to sign (related to enum-enum-conversion.c)
+int f1(enum UE1 E) {
+  return E; // unsigned-warning {{implicit conversion changes signedness: 'enum UE1' to 'int'}}
+}
+
+enum UE1 f2(int E) {
+  return E; // unsigned-warning {{implicit conversion changes signedness: 'int' to 'enum UE1'}}
+}
+
+int f3(enum SE1 E) {
+  return E; // shouldn't warn
+}
+
+enum SE1 f4(int E) {
+  return E; // shouldn't warn
+}
+
+unsigned f5(enum UE1 E) {
+  return E; // signed-warning {{implicit conversion changes signedness: 'enum UE1' to 'unsigned int'}}
+}
+
+enum UE1 f6(unsigned E) {
+  return E; // signed-warning {{implicit conversion changes signedness: 'unsigned int' to 'enum UE1'}}
+}
+
+unsigned f7(enum SE1 E) {
+  return E; // both-warning {{implicit conversion changes signedness: 'enum SE1' to 'unsigned int'}}
+}
+
+enum SE1 f8(unsigned E) {
+  return E; // both-warning {{implicit conversion changes signedness: 'unsigned int' to 'enum SE1'}}
 }
Index: clang/test/Sema/enum-enum-conversion.c
===
--- /dev/null
+++ clang/test/Sema/enum-enum-conversion.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wenum-conversion -verify %s
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wconversion -verify %s
+
+// Signed enums
+enum SE1 { N1 = -1 };
+enum SE2 { N2 = -2 };
+// Unsigned unums
+enum UE1 { P1 };
+enum UE2 { P2 };
+
+enum UE2 f1(enum UE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum UE1' to different enumeration type 'enum UE2'}}
+}
+
+enum SE1 f2(enum UE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum UE1' to different enumeration type 'enum SE1'}}
+}
+
+enum UE1 f3(enum SE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum SE1' to different enumeration type 'enum UE1'}}
+}
+
+enum SE2 f4(enum SE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum SE1' to different enumeration type 'enum SE2'}}
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13534,9 +13534,10 @@
 // Fall through for non-constants to give a sign conversion warning.
   }
 
-  if ((TargetRange.NonNegative && !LikelySourceRange.NonNegative) ||
-  (!TargetRange.NonNegative && LikelySourceRange.NonNegative &&
-   LikelySourceRange.Width == TargetRange.Width)) {
+  if ((!isa(Target) || !isa(Source)) &&
+  ((TargetRange.NonNegative && !LikelySourceRange.NonNegative) ||
+   (!TargetRange.NonNegative && LikelySourceRange.NonNegative &&
+LikelySourceRange.Width == TargetRange.Width))) {
 if (S.SourceMgr.isInSystemMacro(CC))
   return;
 
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -110,6 +110,8 @@
   `_.
 - ``-Wunused-but-set-variable`` now also warns if the variable is only used
   by unary operators.
+- ``-Wenum-conversion`` now warns on conversion of signed enum to un

[PATCH] D123009: [Sema] Enum conversion warning when one signed and other unsigned.

[Micah Weston via Phabricator via cfe-commits]

red1bluelost updated this revision to Diff 426804.
red1bluelost marked 4 inline comments as done.
red1bluelost added a comment.

Addresses comments to improve wording and remove unnecessary code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123009/new/

https://reviews.llvm.org/D123009

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/enum-enum-conversion.c
  clang/test/Sema/enum-sign-conversion.c

Index: clang/test/Sema/enum-sign-conversion.c
===
--- clang/test/Sema/enum-sign-conversion.c
+++ clang/test/Sema/enum-sign-conversion.c
@@ -1,13 +1,47 @@
-// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -verify -DUNSIGNED -Wsign-conversion %s
-// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only -verify -Wsign-conversion %s
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wsign-conversion -verify=unsigned,both %s
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wconversion -verify=unsigned,both %s
+// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only -verify -Wsign-conversion -verify=win32,both %s
 
 // PR35200
-enum X { A,B,C};
+enum X { A, B, C };
 int f(enum X x) {
-#ifdef UNSIGNED
-  return x; // expected-warning {{implicit conversion changes signedness: 'enum X' to 'int'}}
-#else
-  // expected-no-diagnostics
-  return x;
-#endif
+  return x; // unsigned-warning {{implicit conversion changes signedness: 'enum X' to 'int'}}
+}
+
+// Signed enums
+enum SE1 { N1 = -1 };
+// Unsigned unums
+enum UE1 { P1 };
+
+// ensure no regression with enum to sign (related to enum-enum-conversion.c)
+int f1(enum UE1 E) {
+  return E; // unsigned-warning {{implicit conversion changes signedness: 'enum UE1' to 'int'}}
+}
+
+enum UE1 f2(int E) {
+  return E; // unsigned-warning {{implicit conversion changes signedness: 'int' to 'enum UE1'}}
+}
+
+int f3(enum SE1 E) {
+  return E; // shouldn't warn
+}
+
+enum SE1 f4(int E) {
+  return E; // shouldn't warn
+}
+
+unsigned f5(enum UE1 E) {
+  return E; // win32-warning {{implicit conversion changes signedness: 'enum UE1' to 'unsigned int'}}
+}
+
+enum UE1 f6(unsigned E) {
+  return E; // win32-warning {{implicit conversion changes signedness: 'unsigned int' to 'enum UE1'}}
+}
+
+unsigned f7(enum SE1 E) {
+  return E; // both-warning {{implicit conversion changes signedness: 'enum SE1' to 'unsigned int'}}
+}
+
+enum SE1 f8(unsigned E) {
+  return E; // both-warning {{implicit conversion changes signedness: 'unsigned int' to 'enum SE1'}}
 }
Index: clang/test/Sema/enum-enum-conversion.c
===
--- /dev/null
+++ clang/test/Sema/enum-enum-conversion.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wenum-conversion -verify %s
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wconversion -verify %s
+
+// Signed enums
+enum SE1 { N1 = -1 };
+enum SE2 { N2 = -2 };
+// Unsigned unums
+enum UE1 { P1 };
+enum UE2 { P2 };
+
+enum UE2 f1(enum UE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum UE1' to different enumeration type 'enum UE2'}}
+}
+
+enum SE1 f2(enum UE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum UE1' to different enumeration type 'enum SE1'}}
+}
+
+enum UE1 f3(enum SE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum SE1' to different enumeration type 'enum UE1'}}
+}
+
+enum SE2 f4(enum SE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum SE1' to different enumeration type 'enum SE2'}}
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13534,9 +13534,10 @@
 // Fall through for non-constants to give a sign conversion warning.
   }
 
-  if ((TargetRange.NonNegative && !LikelySourceRange.NonNegative) ||
-  (!TargetRange.NonNegative && LikelySourceRange.NonNegative &&
-   LikelySourceRange.Width == TargetRange.Width)) {
+  if ((!isa(Target) || !isa(Source)) &&
+  ((TargetRange.NonNegative && !LikelySourceRange.NonNegative) ||
+   (!TargetRange.NonNegative && LikelySourceRange.NonNegative &&
+LikelySourceRange.Width == TargetRange.Width))) {
 if (S.SourceMgr.isInSystemMacro(CC))
   return;
 
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -110,6 +110,9 @@
   `_.
 - ``-Wunused-but-set-variable`` now also warns if the variable is only used
   by unary operators.
+- ``-Wenum-conversion`` now warns on converting a signed enum of one type to an
+  unsigned enum of a different type (or vice

[PATCH] D123009: [Sema] Enum conversion warning when one signed and other unsigned.

[Micah Weston via Phabricator via cfe-commits]

red1bluelost added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:113-114
   by unary operators.
+- ``-Wenum-conversion`` now warns on conversion of signed enum to unsigned enum
+  and unsigned enum to signed enum rather than ``-Wsign-conversion``.
 

aaron.ballman wrote:
> Minor tweak
Thanks!



Comment at: clang/test/Sema/enum-sign-conversion.c:6-8
+enum X { A,
+ B,
+ C };

aaron.ballman wrote:
> Unrelated formatting change?
Changed back.



Comment at: clang/test/Sema/enum-sign-conversion.c:14-18
+enum SE1 { N1 = -1 };
+enum SE2 { N2 = -2 };
+// Unsigned unums
+enum UE1 { P1 };
+enum UE2 { P2 };

aaron.ballman wrote:
> The second variants are unused.
Good catch, removed.



Comment at: clang/test/Sema/enum-sign-conversion.c:37-43
+unsigned f5(enum UE1 E) {
+  return E; // signed-warning {{implicit conversion changes signedness: 'enum 
UE1' to 'unsigned int'}}
+}
+
+enum UE1 f6(unsigned E) {
+  return E; // signed-warning {{implicit conversion changes signedness: 
'unsigned int' to 'enum UE1'}}
+}

aaron.ballman wrote:
> Shouldn't these not warn as well -- the underlying enum type is unsigned?
This is the case with 'win32' for the ABI. It has enum be signed as default. I 
changed the name of the verify to win32 to that it is easier to see that is the 
case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123009/new/

https://reviews.llvm.org/D123009

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123009: [Sema] Enum conversion warning when one signed and other unsigned.

[Micah Weston via Phabricator via cfe-commits]

red1bluelost updated this revision to Diff 427627.
red1bluelost marked 2 inline comments as done.
red1bluelost added a comment.

Addresses the last couple comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123009/new/

https://reviews.llvm.org/D123009

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/enum-enum-conversion.c
  clang/test/Sema/enum-sign-conversion.c

Index: clang/test/Sema/enum-sign-conversion.c
===
--- clang/test/Sema/enum-sign-conversion.c
+++ clang/test/Sema/enum-sign-conversion.c
@@ -1,13 +1,45 @@
-// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -verify -DUNSIGNED -Wsign-conversion %s
-// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only -verify -Wsign-conversion %s
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wsign-conversion -verify=unsigned,both %s
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wconversion -verify=unsigned,both %s
+// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only -verify -Wsign-conversion -verify=win32,both %s
 
 // PR35200
 enum X { A,B,C};
 int f(enum X x) {
-#ifdef UNSIGNED
-  return x; // expected-warning {{implicit conversion changes signedness: 'enum X' to 'int'}}
-#else
-  // expected-no-diagnostics
-  return x;
-#endif
+  return x; // unsigned-warning {{implicit conversion changes signedness: 'enum X' to 'int'}}
+}
+
+enum SE1 { N1 = -1 }; // Always a signed underlying type.
+enum E1 { P1 };   // Unsigned underlying type except on Windows.
+
+// ensure no regression with enum to sign (related to enum-enum-conversion.c)
+int f1(enum E1 E) {
+  return E; // unsigned-warning {{implicit conversion changes signedness: 'enum E1' to 'int'}}
+}
+
+enum E1 f2(int E) {
+  return E; // unsigned-warning {{implicit conversion changes signedness: 'int' to 'enum E1'}}
+}
+
+int f3(enum SE1 E) {
+  return E; // shouldn't warn
+}
+
+enum SE1 f4(int E) {
+  return E; // shouldn't warn
+}
+
+unsigned f5(enum E1 E) {
+  return E; // win32-warning {{implicit conversion changes signedness: 'enum E1' to 'unsigned int'}}
+}
+
+enum E1 f6(unsigned E) {
+  return E; // win32-warning {{implicit conversion changes signedness: 'unsigned int' to 'enum E1'}}
+}
+
+unsigned f7(enum SE1 E) {
+  return E; // both-warning {{implicit conversion changes signedness: 'enum SE1' to 'unsigned int'}}
+}
+
+enum SE1 f8(unsigned E) {
+  return E; // both-warning {{implicit conversion changes signedness: 'unsigned int' to 'enum SE1'}}
 }
Index: clang/test/Sema/enum-enum-conversion.c
===
--- /dev/null
+++ clang/test/Sema/enum-enum-conversion.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wenum-conversion -verify %s
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wconversion -verify %s
+
+// Signed enums
+enum SE1 { N1 = -1 };
+enum SE2 { N2 = -2 };
+// Unsigned unums
+enum UE1 { P1 };
+enum UE2 { P2 };
+
+enum UE2 f1(enum UE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum UE1' to different enumeration type 'enum UE2'}}
+}
+
+enum SE1 f2(enum UE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum UE1' to different enumeration type 'enum SE1'}}
+}
+
+enum UE1 f3(enum SE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum SE1' to different enumeration type 'enum UE1'}}
+}
+
+enum SE2 f4(enum SE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum SE1' to different enumeration type 'enum SE2'}}
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13534,9 +13534,10 @@
 // Fall through for non-constants to give a sign conversion warning.
   }
 
-  if ((TargetRange.NonNegative && !LikelySourceRange.NonNegative) ||
-  (!TargetRange.NonNegative && LikelySourceRange.NonNegative &&
-   LikelySourceRange.Width == TargetRange.Width)) {
+  if ((!isa(Target) || !isa(Source)) &&
+  ((TargetRange.NonNegative && !LikelySourceRange.NonNegative) ||
+   (!TargetRange.NonNegative && LikelySourceRange.NonNegative &&
+LikelySourceRange.Width == TargetRange.Width))) {
 if (S.SourceMgr.isInSystemMacro(CC))
   return;
 
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -110,6 +110,9 @@
   `_.
 - ``-Wunused-but-set-variable`` now also warns if the variable is only used
   by unary operators.
+- ``-Wenum-conversion`` now warns on converting a signed enum of one type to an
+  unsigned enum of a different type (or vice versa) 

[PATCH] D123009: [Sema] Enum conversion warning when one signed and other unsigned.

[Micah Weston via Phabricator via cfe-commits]

red1bluelost marked an inline comment as done.
red1bluelost added a comment.

If that's all good, could you commit on my behalf. Here is name and email: 
Micah Weston (micahswes...@gmail.com)

Thanks for the great reviews! It was really helpful with learning the frontend 
stuff.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123009/new/

https://reviews.llvm.org/D123009

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137755: [clang-format] Treats &/&& as reference when followed by ',' or ')'.

[Micah Weston via Phabricator via cfe-commits]

red1bluelost created this revision.
red1bluelost added a reviewer: curdeius.
Herald added a project: All.
red1bluelost requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Ran into an issue where function declarations inside function scopes or uses
of sizeof inside a function would treat the && in 'sizeof(Type &&)' as a binary
operator.

Attempt to fix this by assuming reference when followed by ',' or ')'. Also adds
tests for these.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137755

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2132,6 +2132,10 @@
   verifyFormat("int *a = f1();", Style);
   verifyFormat("int &b = f2();", Style);
   verifyFormat("int &&c = f3();", Style);
+  verifyFormat("int f3() { return sizeof(Foo &); }", Style);
+  verifyFormat("int f4() { return sizeof(Foo &&); }", Style);
+  verifyFormat("void f5() { int f6(Foo &, Bar &); }", Style);
+  verifyFormat("void f5() { int f6(Foo &&, Bar &&); }", Style);
   verifyFormat("for (auto a = 0, b = 0; const auto &c : {1, 2, 3})", Style);
   verifyFormat("for (auto a = 0, b = 0; const int &c : {1, 2, 3})", Style);
   verifyFormat("for (auto a = 0, b = 0; const Foo &c : {1, 2, 3})", Style);
@@ -2171,6 +2175,10 @@
   verifyFormat("int* a = f1();", Style);
   verifyFormat("int& b = f2();", Style);
   verifyFormat("int&& c = f3();", Style);
+  verifyFormat("int f3() { return sizeof(Foo&); }", Style);
+  verifyFormat("int f4() { return sizeof(Foo&&); }", Style);
+  verifyFormat("void f5() { int f6(Foo&, Bar&); }", Style);
+  verifyFormat("void f5() { int f6(Foo&&, Bar&&); }", Style);
   verifyFormat("for (auto a = 0, b = 0; const auto& c : {1, 2, 3})", Style);
   verifyFormat("for (auto a = 0, b = 0; const int& c : {1, 2, 3})", Style);
   verifyFormat("for (auto a = 0, b = 0; const Foo& c : {1, 2, 3})", Style);
@@ -2211,6 +2219,10 @@
   verifyFormat("int *a = f1();", Style);
   verifyFormat("int& b = f2();", Style);
   verifyFormat("int&& c = f3();", Style);
+  verifyFormat("int f3() { return sizeof(Foo&); }", Style);
+  verifyFormat("int f4() { return sizeof(Foo&&); }", Style);
+  verifyFormat("void f5() { int f6(Foo&, Bar&); }", Style);
+  verifyFormat("void f5() { int f6(Foo&&, Bar&&); }", Style);
   verifyFormat("for (auto a = 0, b = 0; const Foo *c : {1, 2, 3})", Style);
   verifyFormat("for (int a = 0, b = 0; const Foo *c : {1, 2, 3})", Style);
   verifyFormat("for (int a = 0, b++; const Foo *c : {1, 2, 3})", Style);
@@ -2232,6 +2244,10 @@
   verifyFormat("int* a = f1();", Style);
   verifyFormat("int & b = f2();", Style);
   verifyFormat("int && c = f3();", Style);
+  verifyFormat("int f3() { return sizeof(Foo &); };", Style);
+  verifyFormat("int f4() { return sizeof(Foo &&); };", Style);
+  verifyFormat("void f5() { int f6(Foo &, Bar &); };", Style);
+  verifyFormat("void f5() { int f6(Foo &&, Bar &&); };", Style);
   verifyFormat("for (auto a = 0, b = 0; const auto & c : {1, 2, 3})", Style);
   verifyFormat("for (auto a = 0, b = 0; const int & c : {1, 2, 3})", Style);
   verifyFormat("for (auto a = 0, b = 0; const Foo & c : {1, 2, 3})", Style);
@@ -2268,6 +2284,10 @@
   verifyFormat("int * a = f1();", Style);
   verifyFormat("int &b = f2();", Style);
   verifyFormat("int &&c = f3();", Style);
+  verifyFormat("int f3() { return sizeof(Foo &); };", Style);
+  verifyFormat("int f4() { return sizeof(Foo &&); };", Style);
+  verifyFormat("void f5() { int f6(Foo &, Bar &); };", Style);
+  verifyFormat("void f5() { int f6(Foo &&, Bar &&); };", Style);
   verifyFormat("for (auto a = 0, b = 0; const Foo * c : {1, 2, 3})", Style);
   verifyFormat("for (int a = 0, b = 0; const Foo * c : {1, 2, 3})", Style);
   verifyFormat("for (int a = 0, b++; const Foo * c : {1, 2, 3})", Style);
@@ -8981,7 +9001,7 @@
   "\"long 
literal\");");
   verifyFormat("someFunction(\"Always break between multi-line\"\n"
" \" string literals\",\n"
-   " and, other, parameters);");
+   " also, other, parameters);");
   EXPECT_EQ("fun + \"1243\" /* comment */\n"
 "  \"5678\";",
 format("fun + \"1243\" /* comment */\n"
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2351,7 +2351,8 @@
   return TT_BinaryOperator;
 
 if (!NextToken ||
-NextToken->isOneOf(tok::arrow, tok::equal, tok::kw_noexcept) ||
+NextToken->isOneOf(tok::arrow, tok::equal, tok::kw_noexcept, 
tok::comma,
+   tok::r_paren) ||
 NextToken->canBePoin

[PATCH] D137755: [clang-format] Treats &/&& as reference when followed by ',' or ')'.

[Micah Weston via Phabricator via cfe-commits]

red1bluelost updated this revision to Diff 474615.
red1bluelost added a comment.

Adds tests for the TokenAnnotator as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137755/new/

https://reviews.llvm.org/D137755

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/TokenAnnotatorTest.cpp

Index: clang/unittests/Format/TokenAnnotatorTest.cpp
===
--- clang/unittests/Format/TokenAnnotatorTest.cpp
+++ clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -157,6 +157,24 @@
   Tokens = annotate("if (Foo* Bar = getObj())");
   ASSERT_EQ(Tokens.size(), 11u) << Tokens;
   EXPECT_TOKEN(Tokens[3], tok::star, TT_PointerOrReference);
+
+  Tokens = annotate("int f3() { return sizeof(Foo&); }");
+  ASSERT_EQ(Tokens.size(), 14u) << Tokens;
+  EXPECT_TOKEN(Tokens[9], tok::amp, TT_PointerOrReference);
+
+  Tokens = annotate("int f4() { return sizeof(Foo&&); }");
+  ASSERT_EQ(Tokens.size(), 14u) << Tokens;
+  EXPECT_TOKEN(Tokens[9], tok::ampamp, TT_PointerOrReference);
+
+  Tokens = annotate("void f5() { int f6(Foo&, Bar&); }");
+  ASSERT_EQ(Tokens.size(), 17u) << Tokens;
+  EXPECT_TOKEN(Tokens[9], tok::amp, TT_PointerOrReference);
+  EXPECT_TOKEN(Tokens[12], tok::amp, TT_PointerOrReference);
+
+  Tokens = annotate("void f7() { int f8(Foo&&, Bar&&); }");
+  ASSERT_EQ(Tokens.size(), 17u) << Tokens;
+  EXPECT_TOKEN(Tokens[9], tok::ampamp, TT_PointerOrReference);
+  EXPECT_TOKEN(Tokens[12], tok::ampamp, TT_PointerOrReference);
 }
 
 TEST_F(TokenAnnotatorTest, UnderstandsUsesOfPlusAndMinus) {
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2132,6 +2132,10 @@
   verifyFormat("int *a = f1();", Style);
   verifyFormat("int &b = f2();", Style);
   verifyFormat("int &&c = f3();", Style);
+  verifyFormat("int f3() { return sizeof(Foo &); }", Style);
+  verifyFormat("int f4() { return sizeof(Foo &&); }", Style);
+  verifyFormat("void f5() { int f6(Foo &, Bar &); }", Style);
+  verifyFormat("void f5() { int f6(Foo &&, Bar &&); }", Style);
   verifyFormat("for (auto a = 0, b = 0; const auto &c : {1, 2, 3})", Style);
   verifyFormat("for (auto a = 0, b = 0; const int &c : {1, 2, 3})", Style);
   verifyFormat("for (auto a = 0, b = 0; const Foo &c : {1, 2, 3})", Style);
@@ -2171,6 +2175,10 @@
   verifyFormat("int* a = f1();", Style);
   verifyFormat("int& b = f2();", Style);
   verifyFormat("int&& c = f3();", Style);
+  verifyFormat("int f3() { return sizeof(Foo&); }", Style);
+  verifyFormat("int f4() { return sizeof(Foo&&); }", Style);
+  verifyFormat("void f5() { int f6(Foo&, Bar&); }", Style);
+  verifyFormat("void f5() { int f6(Foo&&, Bar&&); }", Style);
   verifyFormat("for (auto a = 0, b = 0; const auto& c : {1, 2, 3})", Style);
   verifyFormat("for (auto a = 0, b = 0; const int& c : {1, 2, 3})", Style);
   verifyFormat("for (auto a = 0, b = 0; const Foo& c : {1, 2, 3})", Style);
@@ -2211,6 +2219,10 @@
   verifyFormat("int *a = f1();", Style);
   verifyFormat("int& b = f2();", Style);
   verifyFormat("int&& c = f3();", Style);
+  verifyFormat("int f3() { return sizeof(Foo&); }", Style);
+  verifyFormat("int f4() { return sizeof(Foo&&); }", Style);
+  verifyFormat("void f5() { int f6(Foo&, Bar&); }", Style);
+  verifyFormat("void f5() { int f6(Foo&&, Bar&&); }", Style);
   verifyFormat("for (auto a = 0, b = 0; const Foo *c : {1, 2, 3})", Style);
   verifyFormat("for (int a = 0, b = 0; const Foo *c : {1, 2, 3})", Style);
   verifyFormat("for (int a = 0, b++; const Foo *c : {1, 2, 3})", Style);
@@ -2232,6 +2244,10 @@
   verifyFormat("int* a = f1();", Style);
   verifyFormat("int & b = f2();", Style);
   verifyFormat("int && c = f3();", Style);
+  verifyFormat("int f3() { return sizeof(Foo &); }", Style);
+  verifyFormat("int f4() { return sizeof(Foo &&); }", Style);
+  verifyFormat("void f5() { int f6(Foo &, Bar &); }", Style);
+  verifyFormat("void f5() { int f6(Foo &&, Bar &&); }", Style);
   verifyFormat("for (auto a = 0, b = 0; const auto & c : {1, 2, 3})", Style);
   verifyFormat("for (auto a = 0, b = 0; const int & c : {1, 2, 3})", Style);
   verifyFormat("for (auto a = 0, b = 0; const Foo & c : {1, 2, 3})", Style);
@@ -2268,6 +2284,10 @@
   verifyFormat("int * a = f1();", Style);
   verifyFormat("int &b = f2();", Style);
   verifyFormat("int &&c = f3();", Style);
+  verifyFormat("int f3() { return sizeof(Foo &); }", Style);
+  verifyFormat("int f4() { return sizeof(Foo &&); }", Style);
+  verifyFormat("void f5() { int f6(Foo &, Bar &); }", Style);
+  verifyFormat("void f5() { int f6(Foo &&, Bar &&); }", Style);
   verifyFormat("for (auto a = 0, b = 0; const Foo * c : {1, 2, 3})", Style);
   verifyFormat("for (int a = 0, b = 0; const Foo * c : {1, 2, 3})", Style);
   verifyFormat("for (int a = 0, b++; const Foo * 

[PATCH] D137755: [clang-format] Treats &/&& as reference when followed by ',' or ')'.

[Micah Weston via Phabricator via cfe-commits]

red1bluelost marked an inline comment as done.
red1bluelost added a comment.

Made a GitHub issue and addressed the current comments. :)




Comment at: clang/lib/Format/TokenAnnotator.cpp:2355
+NextToken->isOneOf(tok::arrow, tok::equal, tok::kw_noexcept, 
tok::comma,
+   tok::r_paren) ||
 NextToken->canBePointerOrReferenceQualifier() ||

MyDeveloperDay wrote:
> nowadays we add a TokenAnnotator test (they are very easy to write), this 
> lets us ensure its getting annotated correctly. (which is really the bug 
> here), 
> 
> but your tests below are great too!.
Thanks for the suggestion! Didn't realize there were tests for the tokenizer. I 
got those added now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137755/new/

https://reviews.llvm.org/D137755

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137755: [clang-format] Treats &/&& as reference when followed by ',' or ')'.

[Micah Weston via Phabricator via cfe-commits]

red1bluelost marked an inline comment as done.
red1bluelost added a comment.

I noticed that the Premerge seemed ran into an issue with 'InsertBraces'. It 
looks to me that it is unrelated and probably due to the pre-merge machine 
using a pre-15 clang-format (I ran into similar issue on my local machine).

If it is unrelated, could a reviewer commit on my behalf? Name and email is 
"Micah Weston" and micahswes...@gmail.com

Thanks for the helpful reviews!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137755/new/

https://reviews.llvm.org/D137755

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139801: [clang-format] Adds 'friend' to QualifierOrder.

[Micah Weston via Phabricator via cfe-commits]

red1bluelost created this revision.
red1bluelost added a reviewer: MyDeveloperDay.
Herald added a project: All.
red1bluelost requested review of this revision.
Herald added a project: clang.

For cases of defining friend functions, qualifier ordering can allow multiple
positions for the 'friend' token.

  c++
  struct S {
int i;
  
friend constexpr auto operator<=>(const S&, const S&) noexcept = default;
  };



  1. Changes
- Adds `friend` to `getTokenFromQualifier`
- Updates documentation in `Format.h` and `ClangFormatStyleOptions.rst`
- Adds tests to `QualifierFixerTest.cpp`

Testing
---

Ran FormatTest with no errors.

GitHub Issue


https://github.com/llvm/llvm-project/issues/59450


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D139801

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/QualifierAlignmentFixer.cpp
  clang/unittests/Format/QualifierFixerTest.cpp


Index: clang/unittests/Format/QualifierFixerTest.cpp
===
--- clang/unittests/Format/QualifierFixerTest.cpp
+++ clang/unittests/Format/QualifierFixerTest.cpp
@@ -133,6 +133,8 @@
 tok::kw_static);
   
EXPECT_EQ(LeftRightQualifierAlignmentFixer::getTokenFromQualifier("restrict"),
 tok::kw_restrict);
+  EXPECT_EQ(LeftRightQualifierAlignmentFixer::getTokenFromQualifier("friend"),
+tok::kw_friend);
 }
 
 TEST_F(QualifierFixerTest, FailQualifierInvalidConfiguration) {
@@ -196,8 +198,8 @@
 TEST_F(QualifierFixerTest, QualifiersCustomOrder) {
   FormatStyle Style = getLLVMStyle();
   Style.QualifierAlignment = FormatStyle::QAS_Left;
-  Style.QualifierOrder = {"inline", "constexpr", "static",
-  "const",  "volatile",  "type"};
+  Style.QualifierOrder = {"friend", "inline",   "constexpr", "static",
+  "const",  "volatile", "type"};
 
   verifyFormat("const volatile int a;", "const volatile int a;", Style);
   verifyFormat("const volatile int a;", "volatile const int a;", Style);
@@ -216,6 +218,15 @@
   verifyFormat("constexpr static LPINT Bar;", "static constexpr LPINT Bar;",
Style);
   verifyFormat("const const int a;", "const int const a;", Style);
+
+  verifyFormat(
+  "friend constexpr auto operator<=>(const foo &, const foo &) = default;",
+  "constexpr friend auto operator<=>(const foo &, const foo &) = default;",
+  Style);
+  verifyFormat(
+  "friend constexpr bool operator==(const foo &, const foo &) = default;",
+  "constexpr bool friend operator==(const foo &, const foo &) = default;",
+  Style);
 }
 
 TEST_F(QualifierFixerTest, LeftRightQualifier) {
@@ -723,9 +734,10 @@
   ConfiguredTokens.push_back(tok::kw_inline);
   ConfiguredTokens.push_back(tok::kw_restrict);
   ConfiguredTokens.push_back(tok::kw_constexpr);
+  ConfiguredTokens.push_back(tok::kw_friend);
 
-  auto Tokens =
-  annotate("const static inline auto restrict int double long constexpr");
+  auto Tokens = annotate(
+  "const static inline auto restrict int double long constexpr friend");
 
   EXPECT_TRUE(LeftRightQualifierAlignmentFixer::isQualifierOrType(
   Tokens[0], ConfiguredTokens));
@@ -745,6 +757,8 @@
   Tokens[7], ConfiguredTokens));
   EXPECT_TRUE(LeftRightQualifierAlignmentFixer::isQualifierOrType(
   Tokens[8], ConfiguredTokens));
+  EXPECT_TRUE(LeftRightQualifierAlignmentFixer::isQualifierOrType(
+  Tokens[9], ConfiguredTokens));
 
   auto NotTokens = annotate("for while do Foo Bar ");
 
Index: clang/lib/Format/QualifierAlignmentFixer.cpp
===
--- clang/lib/Format/QualifierAlignmentFixer.cpp
+++ clang/lib/Format/QualifierAlignmentFixer.cpp
@@ -414,6 +414,7 @@
   .Case("inline", tok::kw_inline)
   .Case("constexpr", tok::kw_constexpr)
   .Case("restrict", tok::kw_restrict)
+  .Case("friend", tok::kw_friend)
   .Default(tok::identifier);
 }
 
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -2970,6 +2970,7 @@
   ///   * const
   ///   * inline
   ///   * static
+  ///   * friend
   ///   * constexpr
   ///   * volatile
   ///   * restrict
Index: clang/docs/ClangFormatStyleOptions.rst
===
--- clang/docs/ClangFormatStyleOptions.rst
+++ clang/docs/ClangFormatStyleOptions.rst
@@ -3712,6 +3712,7 @@
 * const
 * inline
 * static
+* friend
 * constexpr
 * volatile
 * restrict


Index: clang/unittests/Format/QualifierFixerTest.cpp
===
--- clang/unittests/Format/QualifierFixerTest.cpp
+++ clang/unittests/Format/QualifierFixerTest.cpp
@@ -133,6 +133,8 @@
 tok::kw_static);
   EXPECT_EQ(Le

[PATCH] D139801: [clang-format] Adds 'friend' to QualifierOrder.

[Micah Weston via Phabricator via cfe-commits]

red1bluelost added a comment.

I would always want it first. I agree that `friend` would always go first. Had 
I not accidentally typed in the wrong order I probably wouldn't have know 
`friend` could be in any position.

In the case of putting `friend` always first, would we want a user facing 
option to enable/disable moving it to first, do it always without an user 
option, or something else?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139801/new/

https://reviews.llvm.org/D139801

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139801: [clang-format] Adds 'friend' to QualifierOrder.

[Micah Weston via Phabricator via cfe-commits]

red1bluelost added a comment.

In D139801#3988790 , 
@HazardyKnusperkeks wrote:

> If only the first, I'd say we do that without adding this option.

I'll work on doing this without adding the option. (Just reread and noticed you 
had answered what I asked in my previous comment.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139801/new/

https://reviews.llvm.org/D139801

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139801: [clang-format] Adds 'friend' to QualifierOrder.

[Micah Weston via Phabricator via cfe-commits]

red1bluelost added a comment.

In that case, when someone has free time, could you commit on my behalf (name: 
Micah Weston, email: micahswes...@gmail.com).

Thank you all for the reviews!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139801/new/

https://reviews.llvm.org/D139801

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits