[PATCH] D57852: [clang-tidy] Don't use assignment for value-initialized enums

[Malcolm Parsons via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE353554: [clang-tidy] Don't use assignment for 
value-initialized enums (authored by malcolm.parsons, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D57852?vs=185649&id=186014#toc

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57852

Files:
  clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
  test/clang-tidy/modernize-use-default-member-init-assignment.cpp
  test/clang-tidy/modernize-use-default-member-init.cpp


Index: clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
===
--- clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
+++ clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
@@ -255,17 +255,20 @@
   CharSourceRange InitRange =
   CharSourceRange::getCharRange(LParenEnd, Init->getRParenLoc());
 
+  bool ValueInit = isa(Init->getInit());
+  bool CanAssign = UseAssignment && (!ValueInit || 
!Init->getInit()->getType()->isEnumeralType());
+
   auto Diag =
   diag(Field->getLocation(), "use default member initializer for %0")
   << Field
-  << FixItHint::CreateInsertion(FieldEnd, UseAssignment ? " = " : "{")
+  << FixItHint::CreateInsertion(FieldEnd, CanAssign ? " = " : "{")
   << FixItHint::CreateInsertionFromRange(FieldEnd, InitRange);
 
-  if (UseAssignment && isa(Init->getInit()))
+  if (CanAssign && ValueInit)
 Diag << FixItHint::CreateInsertion(
 FieldEnd, getValueOfValueInit(Init->getInit()->getType()));
 
-  if (!UseAssignment)
+  if (!CanAssign)
 Diag << FixItHint::CreateInsertion(FieldEnd, "}");
 
   Diag << FixItHint::CreateRemoval(Init->getSourceRange());
Index: test/clang-tidy/modernize-use-default-member-init.cpp
===
--- test/clang-tidy/modernize-use-default-member-init.cpp
+++ test/clang-tidy/modernize-use-default-member-init.cpp
@@ -165,6 +165,14 @@
   // CHECK-FIXES: Enum e{Foo};
 };
 
+struct PositiveValueEnum {
+  PositiveValueEnum() : e() {}
+  // CHECK-FIXES: PositiveValueEnum()  {}
+  Enum e;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use default member initializer 
for 'e'
+  // CHECK-FIXES: Enum e{};
+};
+
 struct PositiveString {
   PositiveString() : s("foo") {}
   // CHECK-FIXES: PositiveString()  {}
Index: test/clang-tidy/modernize-use-default-member-init-assignment.cpp
===
--- test/clang-tidy/modernize-use-default-member-init-assignment.cpp
+++ test/clang-tidy/modernize-use-default-member-init-assignment.cpp
@@ -166,6 +166,14 @@
   // CHECK-FIXES: Enum e = Foo;
 };
 
+struct PositiveValueEnum {
+  PositiveValueEnum() : e() {}
+  // CHECK-FIXES: PositiveValueEnum()  {}
+  Enum e;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use default member initializer 
for 'e'
+  // CHECK-FIXES: Enum e{};
+};
+
 struct PositiveString {
   PositiveString() : s("foo") {}
   // CHECK-FIXES: PositiveString()  {}


Index: clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
===
--- clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
+++ clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
@@ -255,17 +255,20 @@
   CharSourceRange InitRange =
   CharSourceRange::getCharRange(LParenEnd, Init->getRParenLoc());
 
+  bool ValueInit = isa(Init->getInit());
+  bool CanAssign = UseAssignment && (!ValueInit || !Init->getInit()->getType()->isEnumeralType());
+
   auto Diag =
   diag(Field->getLocation(), "use default member initializer for %0")
   << Field
-  << FixItHint::CreateInsertion(FieldEnd, UseAssignment ? " = " : "{")
+  << FixItHint::CreateInsertion(FieldEnd, CanAssign ? " = " : "{")
   << FixItHint::CreateInsertionFromRange(FieldEnd, InitRange);
 
-  if (UseAssignment && isa(Init->getInit()))
+  if (CanAssign && ValueInit)
 Diag << FixItHint::CreateInsertion(
 FieldEnd, getValueOfValueInit(Init->getInit()->getType()));
 
-  if (!UseAssignment)
+  if (!CanAssign)
 Diag << FixItHint::CreateInsertion(FieldEnd, "}");
 
   Diag << FixItHint::CreateRemoval(Init->getSourceRange());
Index: test/clang-tidy/modernize-use-default-member-init.cpp
===
--- test/clang-tidy/modernize-use-default-member-init.cpp
+++ test/clang-tidy/modernize-use-default-member-init.cpp
@@ -165,6 +165,14 @@
   // CHECK-FIXES: Enum e{Foo};
 };
 
+struct PositiveValueEnum {
+  PositiveValueEnum() : e() {}
+  // CHECK-FIXES: PositiveValueEnum()  {}
+  Enum e;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use default member initializer for 'e'
+  // CHECK-FIXES: Enum e{};
+};
+
 struct PositiveString {
   PositiveString() : s("foo") {}
   // CHECK-FIXES: PositiveString()  {}
Index: test/clang-tidy/modernize-use-default-member-init-assignment

[PATCH] D57852: [clang-tidy] Don't use assignment for value-initialized enums

[Jonas Toth via Phabricator via cfe-commits]

JonasToth accepted this revision.
JonasToth added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57852



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

[PATCH] D57852: [clang-tidy] Don't use assignment for value-initialized enums

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons marked an inline comment as done.
malcolm.parsons added a comment.

In D57852#1388526 , @JonasToth wrote:

> How are the semantics for `enum class` in this case?


No enumerators are present in the initialisation or the fixit, so there is no 
difference.




Comment at: 
clang-tools-extra/test/clang-tidy/modernize-use-default-member-init-assignment.cpp:172
+  // CHECK-FIXES: PositiveValueEnum()  {}
+  Enum e;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use default member initializer 
for 'e'

JonasToth wrote:
> What happens for the case `enum Enum { Foo = 3 }; /* ... */ : e() /* ... */`, 
> is that even well formed?
> I feel a testcase along those lines is missing.
There are no errors or warnings from gcc or clang for that code.
Adding a testcase wouldn't increase coverage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57852



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

[PATCH] D57852: [clang-tidy] Don't use assignment for value-initialized enums

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

How are the semantics for `enum class` in this case?




Comment at: 
clang-tools-extra/test/clang-tidy/modernize-use-default-member-init-assignment.cpp:172
+  // CHECK-FIXES: PositiveValueEnum()  {}
+  Enum e;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use default member initializer 
for 'e'

What happens for the case `enum Enum { Foo = 3 }; /* ... */ : e() /* ... */`, 
is that even well formed?
I feel a testcase along those lines is missing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57852



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

[PATCH] D57852: [clang-tidy] Don't use assignment for value-initialized enums

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons created this revision.
malcolm.parsons added reviewers: aaron.ballman, alexfh, JonasToth.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.

The modernize-use-default-member-init check crashes when trying to
create an assignment value for a value-initialized enum because it isn't a
BuiltinType.
An enum cannot be initialized by assigning 0 to it unless a cast is added.
It could be initialized with an enumerator with the value 0, but there might not
be one.
Avoid these issues by ignoring the UseAssignment setting for value-initialized
enums.

Fixes PR35050.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D57852

Files:
  clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
  
clang-tools-extra/test/clang-tidy/modernize-use-default-member-init-assignment.cpp
  clang-tools-extra/test/clang-tidy/modernize-use-default-member-init.cpp


Index: clang-tools-extra/test/clang-tidy/modernize-use-default-member-init.cpp
===
--- clang-tools-extra/test/clang-tidy/modernize-use-default-member-init.cpp
+++ clang-tools-extra/test/clang-tidy/modernize-use-default-member-init.cpp
@@ -165,6 +165,14 @@
   // CHECK-FIXES: Enum e{Foo};
 };
 
+struct PositiveValueEnum {
+  PositiveValueEnum() : e() {}
+  // CHECK-FIXES: PositiveValueEnum()  {}
+  Enum e;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use default member initializer 
for 'e'
+  // CHECK-FIXES: Enum e{};
+};
+
 struct PositiveString {
   PositiveString() : s("foo") {}
   // CHECK-FIXES: PositiveString()  {}
Index: 
clang-tools-extra/test/clang-tidy/modernize-use-default-member-init-assignment.cpp
===
--- 
clang-tools-extra/test/clang-tidy/modernize-use-default-member-init-assignment.cpp
+++ 
clang-tools-extra/test/clang-tidy/modernize-use-default-member-init-assignment.cpp
@@ -166,6 +166,14 @@
   // CHECK-FIXES: Enum e = Foo;
 };
 
+struct PositiveValueEnum {
+  PositiveValueEnum() : e() {}
+  // CHECK-FIXES: PositiveValueEnum()  {}
+  Enum e;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use default member initializer 
for 'e'
+  // CHECK-FIXES: Enum e{};
+};
+
 struct PositiveString {
   PositiveString() : s("foo") {}
   // CHECK-FIXES: PositiveString()  {}
Index: clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
@@ -255,17 +255,20 @@
   CharSourceRange InitRange =
   CharSourceRange::getCharRange(LParenEnd, Init->getRParenLoc());
 
+  bool ValueInit = isa(Init->getInit());
+  bool CanAssign = UseAssignment && (!ValueInit || 
!Init->getInit()->getType()->isEnumeralType());
+
   auto Diag =
   diag(Field->getLocation(), "use default member initializer for %0")
   << Field
-  << FixItHint::CreateInsertion(FieldEnd, UseAssignment ? " = " : "{")
+  << FixItHint::CreateInsertion(FieldEnd, CanAssign ? " = " : "{")
   << FixItHint::CreateInsertionFromRange(FieldEnd, InitRange);
 
-  if (UseAssignment && isa(Init->getInit()))
+  if (CanAssign && ValueInit)
 Diag << FixItHint::CreateInsertion(
 FieldEnd, getValueOfValueInit(Init->getInit()->getType()));
 
-  if (!UseAssignment)
+  if (!CanAssign)
 Diag << FixItHint::CreateInsertion(FieldEnd, "}");
 
   Diag << FixItHint::CreateRemoval(Init->getSourceRange());


Index: clang-tools-extra/test/clang-tidy/modernize-use-default-member-init.cpp
===
--- clang-tools-extra/test/clang-tidy/modernize-use-default-member-init.cpp
+++ clang-tools-extra/test/clang-tidy/modernize-use-default-member-init.cpp
@@ -165,6 +165,14 @@
   // CHECK-FIXES: Enum e{Foo};
 };
 
+struct PositiveValueEnum {
+  PositiveValueEnum() : e() {}
+  // CHECK-FIXES: PositiveValueEnum()  {}
+  Enum e;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use default member initializer for 'e'
+  // CHECK-FIXES: Enum e{};
+};
+
 struct PositiveString {
   PositiveString() : s("foo") {}
   // CHECK-FIXES: PositiveString()  {}
Index: clang-tools-extra/test/clang-tidy/modernize-use-default-member-init-assignment.cpp
===
--- clang-tools-extra/test/clang-tidy/modernize-use-default-member-init-assignment.cpp
+++ clang-tools-extra/test/clang-tidy/modernize-use-default-member-init-assignment.cpp
@@ -166,6 +166,14 @@
   // CHECK-FIXES: Enum e = Foo;
 };
 
+struct PositiveValueEnum {
+  PositiveValueEnum() : e() {}
+  // CHECK-FIXES: PositiveValueEnum()  {}
+  Enum e;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use default member initializer for 'e'
+  // CHECK-FIXES: Enum e{};
+};
+
 struct PositiveString {
   PositiveString() : s("foo") {}
   // CHECK-FIXES: PositiveString()  {