[PATCH] D76996: [analyzer] Improve PlacementNewChecker

[Karasev Nikita via Phabricator via cfe-commits]

f00kat updated this revision to Diff 255039.
f00kat added a comment.

1. Maybe build fix
2. Added tests for nested arrays of structures
3. Fixed bugs in implementation for ElementRegion cases


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

https://reviews.llvm.org/D76996

Files:
  clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
  clang/test/Analysis/placement-new.cpp

Index: clang/test/Analysis/placement-new.cpp
===
--- clang/test/Analysis/placement-new.cpp
+++ clang/test/Analysis/placement-new.cpp
@@ -155,3 +155,312 @@
   (void)dp;
 }
 } // namespace testHierarchy
+
+namespace testArrayTypesAllocation {
+void f1() {
+  struct S {
+short a;
+  };
+
+  // bad (not enough space).
+  const unsigned N = 32;
+  alignas(S) unsigned char buffer1[sizeof(S) * N]; // expected-note {{'buffer1' initialized here}}
+  ::new (buffer1) S[N];// expected-warning{{Storage provided to placement new is only 64 bytes, whereas the allocated array type requires more space for internal needs}} expected-note 1 {{}}
+}
+
+void f2() {
+  struct S {
+short a;
+  };
+
+  // maybe ok but we need to warn.
+  const unsigned N = 32;
+  alignas(S) unsigned char buffer2[sizeof(S) * N + sizeof(int)]; // expected-note {{'buffer2' initialized here}}
+  ::new (buffer2) S[N];  // expected-warning{{68 bytes is possibly not enough for array allocation which requires 64 bytes. Current overhead requires the size of 4 bytes}} expected-note 1 {{}}
+}
+} // namespace testArrayTypesAllocation
+
+namespace testStructAlign {
+void f1() {
+  struct X {
+char a[9];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to char).
+  ::new (&Xi.a) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f2() {
+  struct X {
+char a;
+char b;
+long c;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+
+void f3() {
+  struct X {
+char a;
+char b;
+long c;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to long but field 'b' is aligned to 1 because of its offset)
+  ::new (&Xi.b) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f4() {
+  struct X {
+char a;
+struct alignas(alignof(short)) Y {
+  char b;
+  char c;
+} y;
+long d;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad. 'b' is aligned to short
+  ::new (&Xi.y.b) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f5() {
+  short b[10]; // expected-note {{'b' initialized here}}
+
+  ::new (&b) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f6() {
+  short b[10]; // expected-note {{'b' initialized here}}
+
+  // bad (same as previous but checks ElementRegion case)
+  ::new (&b[0]) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f7() {
+  short b[10];
+
+  // ok. 2(short align) + 3*2(index '3' offset)
+  ::new (&b[3]) long;
+}
+
+void f8() {
+  short b[10]; // expected-note {{'b' initialized here}}
+
+  // bad. 2(short align) + 2*2(index '2' offset)
+  ::new (&b[2]) long; // expected-warning{{Storage type is aligned to 6 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f9() {
+  struct X {
+char a;
+alignas(alignof(short)) char b[20];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // ok 2(custom align) + 6*1(index '6' offset)
+  ::new (&Xi.b[6]) long;
+
+  // bad 2(custom align) + 1*1(index '1' offset)
+  ::new (&Xi.b[1]) long; // expected-warning{{Storage type is aligned to 3 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f10() {
+  struct X {
+char a[8];
+alignas(2) char b;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to 2).
+  ::new (&Xi.a) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f11() {
+  struct X {
+char a;
+char b;
+struct Y {
+  long c;
+} d;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+
+void f12() {
+  struct alignas(alignof(long)) X {
+char a;
+char b;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+
+void test13() {
+  struct Y {
+char a[10];
+  };
+
+  struct X {
+Y b[10];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad. X,A are aligned to 'char'
+  ::new (&Xi.b[0].a) long;  // expected-warning{{Storage type is

[PATCH] D76996: [analyzer] Improve PlacementNewChecker

[Karasev Nikita via Phabricator via cfe-commits]

f00kat marked 4 inline comments as done.
f00kat added a comment.

Ping? :)




Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:189
+  if (const MemRegion *MRegion = PlaceVal.getAsRegion()) {
+if (const ElementRegion *TheElementRegion =
+MRegion->getAs()) {

NoQ wrote:
> NoQ wrote:
> > The sequence of `FieldRegion`s and `ElementRegion`s on top of a base region 
> > may be arbitrary: `var.a[0].b[1][2].c.d[3]` etc.
> > 
> > I'd rather unwrap those regions one-by-one in a loop and look at the 
> > alignment of each layer.
> Alternatively, just decompose the whole region into base region and offset 
> and see if base region has the necessary alignment and the offset is 
> divisible by the necessary alignment.
> The sequence of FieldRegions and ElementRegions on top of a base region may 
> be arbitrary: var.a[0].b[1][2].c.d[3] etc.

But i think(hope) I already do this and even have tests for this cases. For 
example
```void test22() {
  struct alignas(alignof(short)) Z {
char p;
char c[10];
  };

  struct Y {
char p;
Z b[10];
  };

  struct X {
Y a[10];
  } Xi; // expected-note {{'Xi' initialized here}}

  // ok. 2(X align) + 1 (offset Y.p) + 1(align Z.p to 'short') + 1(offset Z.p) 
+ 3(index)
  ::new (&Xi.a[0].b[0].c[3]) long;
}```

Cases with multidimensional arrays will also be handled correctly because 
method 'TheElementRegion->getAsArrayOffset()' calculates the offset for 
multidimensional arrays
```void testXX() {
struct Y {
char p;
char b[10][10];
};

struct X {
Y a[10];
} Xi;

::new (&Xi.a[0].b[0][0]) long;
}```

I can explain the code below for ElementRegion if needed.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:189
+  if (const MemRegion *MRegion = PlaceVal.getAsRegion()) {
+if (const ElementRegion *TheElementRegion =
+MRegion->getAs()) {

f00kat wrote:
> NoQ wrote:
> > NoQ wrote:
> > > The sequence of `FieldRegion`s and `ElementRegion`s on top of a base 
> > > region may be arbitrary: `var.a[0].b[1][2].c.d[3]` etc.
> > > 
> > > I'd rather unwrap those regions one-by-one in a loop and look at the 
> > > alignment of each layer.
> > Alternatively, just decompose the whole region into base region and offset 
> > and see if base region has the necessary alignment and the offset is 
> > divisible by the necessary alignment.
> > The sequence of FieldRegions and ElementRegions on top of a base region may 
> > be arbitrary: var.a[0].b[1][2].c.d[3] etc.
> 
> But i think(hope) I already do this and even have tests for this cases. For 
> example
> ```void test22() {
>   struct alignas(alignof(short)) Z {
> char p;
> char c[10];
>   };
> 
>   struct Y {
> char p;
> Z b[10];
>   };
> 
>   struct X {
> Y a[10];
>   } Xi; // expected-note {{'Xi' initialized here}}
> 
>   // ok. 2(X align) + 1 (offset Y.p) + 1(align Z.p to 'short') + 1(offset 
> Z.p) + 3(index)
>   ::new (&Xi.a[0].b[0].c[3]) long;
> }```
> 
> Cases with multidimensional arrays will also be handled correctly because 
> method 'TheElementRegion->getAsArrayOffset()' calculates the offset for 
> multidimensional arrays
> ```void testXX() {
>   struct Y {
>   char p;
>   char b[10][10];
>   };
> 
>   struct X {
>   Y a[10];
>   } Xi;
> 
>   ::new (&Xi.a[0].b[0][0]) long;
> }```
> 
> I can explain the code below for ElementRegion if needed.
?



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:25
 public:
   void checkPreStmt(const CXXNewExpr *NE, CheckerContext &C) const;
 

NoQ wrote:
> Before i forget: Ideally @martong should have subscribed to [[ 
> https://clang.llvm.org/doxygen/classclang_1_1ento_1_1CheckerDocumentation.html#a7fdb3b5ff726f4c5e782cef0d59c01ad
>  | `checkNewAllocator` ]] because it fires before the construct-expression 
> whereas this callback fires after construct-expression which is too late as 
> the UB we're trying to catch has occured much earlier.
Ops, I forgot about it. Will be fixed soon.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:25
 public:
   void checkPreStmt(const CXXNewExpr *NE, CheckerContext &C) const;
 

f00kat wrote:
> NoQ wrote:
> > Before i forget: Ideally @martong should have subscribed to [[ 
> > https://clang.llvm.org/doxygen/classclang_1_1ento_1_1CheckerDocumentation.html#a7fdb3b5ff726f4c5e782cef0d59c01ad
> >  | `checkNewAllocator` ]] because it fires before the construct-expression 
> > whereas this callback fires after construct-expression which is too late as 
> > the UB we're trying to catch has occured much earlier.
> Ops, I forgot about it. Will be fixed soon.
When I use checkNewAllocator instead of check::PreStmt an error 
occures in method PathDiagnosticBuilder::generate.
I

[PATCH] D76996: [analyzer] Improve PlacementNewChecker

[Karasev Nikita via Phabricator via cfe-commits]

f00kat updated this revision to Diff 259499.
f00kat added a comment.

1. Rewroted ElementRegion processing and fixed tests for this cases.
2. Simplified the code a bit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76996

Files:
  clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
  clang/test/Analysis/placement-new.cpp

Index: clang/test/Analysis/placement-new.cpp
===
--- clang/test/Analysis/placement-new.cpp
+++ clang/test/Analysis/placement-new.cpp
@@ -155,3 +155,309 @@
   (void)dp;
 }
 } // namespace testHierarchy
+
+namespace testArrayTypesAllocation {
+void f1() {
+  struct S {
+short a;
+  };
+
+  // bad (not enough space).
+  const unsigned N = 32;
+  alignas(S) unsigned char buffer1[sizeof(S) * N]; // expected-note {{'buffer1' initialized here}}
+  ::new (buffer1) S[N];// expected-warning{{Storage provided to placement new is only 64 bytes, whereas the allocated array type requires more space for internal needs}} expected-note 1 {{}}
+}
+
+void f2() {
+  struct S {
+short a;
+  };
+
+  // maybe ok but we need to warn.
+  const unsigned N = 32;
+  alignas(S) unsigned char buffer2[sizeof(S) * N + sizeof(int)]; // expected-note {{'buffer2' initialized here}}
+  ::new (buffer2) S[N];  // expected-warning{{68 bytes is possibly not enough for array allocation which requires 64 bytes. Current overhead requires the size of 4 bytes}} expected-note 1 {{}}
+}
+} // namespace testArrayTypesAllocation
+
+namespace testStructAlign {
+void f1() {
+  struct X {
+char a[9];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to char).
+  ::new (&Xi.a) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f2() {
+  struct X {
+char a;
+char b;
+long c;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+
+void f3() {
+  struct X {
+char a;
+char b;
+long c;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to long but field 'b' is aligned to 1 because of its offset)
+  ::new (&Xi.b) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f4() {
+  struct X {
+char a;
+struct alignas(alignof(short)) Y {
+  char b;
+  char c;
+} y;
+long d;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad. 'b' is aligned to short
+  ::new (&Xi.y.b) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f5() {
+  short b[10]; // expected-note {{'b' initialized here}}
+
+  ::new (&b) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f6() {
+  short b[10]; // expected-note {{'b' initialized here}}
+
+  // bad (same as previous but checks ElementRegion case)
+  ::new (&b[0]) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f7() {
+  alignas(alignof(long)) short b[10];
+
+  // ok. aligned to long(ok). offset 4*2(ok)
+  ::new (&b[4]) long;
+}
+
+void f8() {
+  alignas(alignof(long)) short b[10]; // expected-note {{'b' initialized here}}
+
+  // ok. aligned to long(ok). offset 3*2(ok)
+  ::new (&b[3]) long; // expected-warning{{Storage type is aligned to 6 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f9() {
+  struct X {
+char a;
+alignas(alignof(long)) char b[20];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // ok. aligned to long(ok). offset 8*1(ok)
+  ::new (&Xi.b[8]) long;
+
+  // bad. aligned to long(ok). offset 1*1(ok)
+  ::new (&Xi.b[1]) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f10() {
+  struct X {
+char a[8];
+alignas(2) char b;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to 2).
+  ::new (&Xi.a) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f11() {
+  struct X {
+char a;
+char b;
+struct Y {
+  long c;
+} d;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+
+void f12() {
+  struct alignas(alignof(long)) X {
+char a;
+char b;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+
+void test13() {
+  struct Y {
+char a[10];
+  };
+
+  struct X {
+Y b[10];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad. X,A are aligned to 'char'
+  ::new (&Xi.b[0

[PATCH] D76996: [analyzer] Improve PlacementNewChecker

[Karasev Nikita via Phabricator via cfe-commits]

f00kat updated this revision to Diff 259636.
f00kat marked 4 inline comments as done.
f00kat added a comment.

1. Refactoring
2. Build fix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76996

Files:
  clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
  clang/test/Analysis/placement-new.cpp

Index: clang/test/Analysis/placement-new.cpp
===
--- clang/test/Analysis/placement-new.cpp
+++ clang/test/Analysis/placement-new.cpp
@@ -155,3 +155,309 @@
   (void)dp;
 }
 } // namespace testHierarchy
+
+namespace testArrayTypesAllocation {
+void f1() {
+  struct S {
+short a;
+  };
+
+  // bad (not enough space).
+  const unsigned N = 32;
+  alignas(S) unsigned char buffer1[sizeof(S) * N]; // expected-note {{'buffer1' initialized here}}
+  ::new (buffer1) S[N];// expected-warning{{Storage provided to placement new is only 64 bytes, whereas the allocated array type requires more space for internal needs}} expected-note 1 {{}}
+}
+
+void f2() {
+  struct S {
+short a;
+  };
+
+  // maybe ok but we need to warn.
+  const unsigned N = 32;
+  alignas(S) unsigned char buffer2[sizeof(S) * N + sizeof(int)]; // expected-note {{'buffer2' initialized here}}
+  ::new (buffer2) S[N];  // expected-warning{{68 bytes is possibly not enough for array allocation which requires 64 bytes. Current overhead requires the size of 4 bytes}} expected-note 1 {{}}
+}
+} // namespace testArrayTypesAllocation
+
+namespace testStructAlign {
+void f1() {
+  struct X {
+char a[9];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to char).
+  ::new (&Xi.a) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f2() {
+  struct X {
+char a;
+char b;
+long c;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+
+void f3() {
+  struct X {
+char a;
+char b;
+long c;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to long but field 'b' is aligned to 1 because of its offset)
+  ::new (&Xi.b) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f4() {
+  struct X {
+char a;
+struct alignas(alignof(short)) Y {
+  char b;
+  char c;
+} y;
+long d;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad. 'b' is aligned to short
+  ::new (&Xi.y.b) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f5() {
+  short b[10]; // expected-note {{'b' initialized here}}
+
+  ::new (&b) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f6() {
+  short b[10]; // expected-note {{'b' initialized here}}
+
+  // bad (same as previous but checks ElementRegion case)
+  ::new (&b[0]) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f7() {
+  alignas(alignof(long)) short b[10];
+
+  // ok. aligned to long(ok). offset 4*2(ok)
+  ::new (&b[4]) long;
+}
+
+void f8() {
+  alignas(alignof(long)) short b[10]; // expected-note {{'b' initialized here}}
+
+  // ok. aligned to long(ok). offset 3*2(ok)
+  ::new (&b[3]) long; // expected-warning{{Storage type is aligned to 6 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f9() {
+  struct X {
+char a;
+alignas(alignof(long)) char b[20];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // ok. aligned to long(ok). offset 8*1(ok)
+  ::new (&Xi.b[8]) long;
+
+  // bad. aligned to long(ok). offset 1*1(ok)
+  ::new (&Xi.b[1]) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f10() {
+  struct X {
+char a[8];
+alignas(2) char b;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to 2).
+  ::new (&Xi.a) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f11() {
+  struct X {
+char a;
+char b;
+struct Y {
+  long c;
+} d;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+
+void f12() {
+  struct alignas(alignof(long)) X {
+char a;
+char b;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+
+void test13() {
+  struct Y {
+char a[10];
+  };
+
+  struct X {
+Y b[10];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad. X,A are aligned to 'char'
+  ::new (&Xi.b[0].a) long; // expected-warning

[PATCH] D76996: [analyzer] Improve PlacementNewChecker

[Karasev Nikita via Phabricator via cfe-commits]

f00kat updated this revision to Diff 259667.
f00kat added a comment.

1. Build fix


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

https://reviews.llvm.org/D76996

Files:
  clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
  clang/test/Analysis/placement-new.cpp

Index: clang/test/Analysis/placement-new.cpp
===
--- clang/test/Analysis/placement-new.cpp
+++ clang/test/Analysis/placement-new.cpp
@@ -155,3 +155,309 @@
   (void)dp;
 }
 } // namespace testHierarchy
+
+namespace testArrayTypesAllocation {
+void f1() {
+  struct S {
+short a;
+  };
+
+  // bad (not enough space).
+  const unsigned N = 32;
+  alignas(S) unsigned char buffer1[sizeof(S) * N]; // expected-note {{'buffer1' initialized here}}
+  ::new (buffer1) S[N];// expected-warning{{Storage provided to placement new is only 64 bytes, whereas the allocated array type requires more space for internal needs}} expected-note 1 {{}}
+}
+
+void f2() {
+  struct S {
+short a;
+  };
+
+  // maybe ok but we need to warn.
+  const unsigned N = 32;
+  alignas(S) unsigned char buffer2[sizeof(S) * N + sizeof(int)]; // expected-note {{'buffer2' initialized here}}
+  ::new (buffer2) S[N];  // expected-warning{{68 bytes is possibly not enough for array allocation which requires 64 bytes. Current overhead requires the size of 4 bytes}} expected-note 1 {{}}
+}
+} // namespace testArrayTypesAllocation
+
+namespace testStructAlign {
+void f1() {
+  struct X {
+char a[9];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to char).
+  ::new (&Xi.a) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f2() {
+  struct X {
+char a;
+char b;
+long c;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+
+void f3() {
+  struct X {
+char a;
+char b;
+long c;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to long but field 'b' is aligned to 1 because of its offset)
+  ::new (&Xi.b) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f4() {
+  struct X {
+char a;
+struct alignas(alignof(short)) Y {
+  char b;
+  char c;
+} y;
+long d;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad. 'b' is aligned to short
+  ::new (&Xi.y.b) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f5() {
+  short b[10]; // expected-note {{'b' initialized here}}
+
+  ::new (&b) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f6() {
+  short b[10]; // expected-note {{'b' initialized here}}
+
+  // bad (same as previous but checks ElementRegion case)
+  ::new (&b[0]) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f7() {
+  alignas(alignof(long)) short b[10];
+
+  // ok. aligned to long(ok). offset 4*2(ok)
+  ::new (&b[4]) long;
+}
+
+void f8() {
+  alignas(alignof(long)) short b[10]; // expected-note {{'b' initialized here}}
+
+  // ok. aligned to long(ok). offset 3*2(ok)
+  ::new (&b[3]) long; // expected-warning{{Storage type is aligned to 6 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f9() {
+  struct X {
+char a;
+alignas(alignof(long)) char b[20];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // ok. aligned to long(ok). offset 8*1(ok)
+  ::new (&Xi.b[8]) long;
+
+  // bad. aligned to long(ok). offset 1*1(ok)
+  ::new (&Xi.b[1]) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f10() {
+  struct X {
+char a[8];
+alignas(2) char b;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to 2).
+  ::new (&Xi.a) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f11() {
+  struct X {
+char a;
+char b;
+struct Y {
+  long c;
+} d;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+
+void f12() {
+  struct alignas(alignof(long)) X {
+char a;
+char b;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+
+void test13() {
+  struct Y {
+char a[10];
+  };
+
+  struct X {
+Y b[10];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad. X,A are aligned to 'char'
+  ::new (&Xi.b[0].a) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1

[PATCH] D76996: [analyzer] Improve PlacementNewChecker

[Karasev Nikita via Phabricator via cfe-commits]

f00kat added a comment.

In D76996#2017572 , @martong wrote:

> > ... This draws a pattern that we should recursively descend down to the top 
> > most base region. I.e. the different check*RegionAlign methods should call 
> > into each other until we reach the top level base region.
>
>
>
> > The observation here is that the alignment of a region can be correct only 
> > if we can prove that its base region is aligned properly (and other 
> > requirements, e.g. the offset is divisible). But the base region may have 
> > another base region and we have to prove the alignment correctness to that 
> > as well.
>
> This could be an issue not just with alignment but maybe with the size as 
> well, I am not sure if we handle the offset properly in compound cases like 
> this: `Xi.b[0].a[1][6]`.
>
> Even though the above issue is still not investigated/handled, I think this 
> patch is now acceptable because seems like most of the practical cases are 
> handled. We could further investigate the concern and improve in a follow-up 
> patch.
>  I'd like to see this landed and thanks for your work!


Thanks for feedback!

I still have no rights to push in the repo so if you think that it is 
acceptable could you commit it please?




Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:189
+  if (const MemRegion *MRegion = PlaceVal.getAsRegion()) {
+if (const ElementRegion *TheElementRegion =
+MRegion->getAs()) {

NoQ wrote:
> martong wrote:
> > f00kat wrote:
> > > f00kat wrote:
> > > > NoQ wrote:
> > > > > NoQ wrote:
> > > > > > The sequence of `FieldRegion`s and `ElementRegion`s on top of a 
> > > > > > base region may be arbitrary: `var.a[0].b[1][2].c.d[3]` etc.
> > > > > > 
> > > > > > I'd rather unwrap those regions one-by-one in a loop and look at 
> > > > > > the alignment of each layer.
> > > > > Alternatively, just decompose the whole region into base region and 
> > > > > offset and see if base region has the necessary alignment and the 
> > > > > offset is divisible by the necessary alignment.
> > > > > The sequence of FieldRegions and ElementRegions on top of a base 
> > > > > region may be arbitrary: var.a[0].b[1][2].c.d[3] etc.
> > > > 
> > > > But i think(hope) I already do this and even have tests for this cases. 
> > > > For example
> > > > ```void test22() {
> > > >   struct alignas(alignof(short)) Z {
> > > > char p;
> > > > char c[10];
> > > >   };
> > > > 
> > > >   struct Y {
> > > > char p;
> > > > Z b[10];
> > > >   };
> > > > 
> > > >   struct X {
> > > > Y a[10];
> > > >   } Xi; // expected-note {{'Xi' initialized here}}
> > > > 
> > > >   // ok. 2(X align) + 1 (offset Y.p) + 1(align Z.p to 'short') + 
> > > > 1(offset Z.p) + 3(index)
> > > >   ::new (&Xi.a[0].b[0].c[3]) long;
> > > > }```
> > > > 
> > > > Cases with multidimensional arrays will also be handled correctly 
> > > > because method 'TheElementRegion->getAsArrayOffset()' calculates the 
> > > > offset for multidimensional arrays
> > > > ```void testXX() {
> > > > struct Y {
> > > > char p;
> > > > char b[10][10];
> > > > };
> > > > 
> > > > struct X {
> > > > Y a[10];
> > > > } Xi;
> > > > 
> > > > ::new (&Xi.a[0].b[0][0]) long;
> > > > }```
> > > > 
> > > > I can explain the code below for ElementRegion if needed.
> > > ?
> > Yeah, the tests are convincing and I think that you are handling the 
> > regions well.
> > 
> > On the other hand, this code is getting really complex, we should make it 
> > easier to read and understand.
> > E.g. `FieldOffsetValue` should be explained more, is it the offset started 
> > from the the start address of the multidimensional array, or it is just the 
> > offset from one element's start address?
> > Also, you have two variables named as `Offset`. They are offsets from which 
> > starting address? Perhaps we should have in the comments a running example, 
> > maybe for `&Xi.a[0].b[0][0]`? I mean is `FieldOffseValue` is standing for 
> > `b` or for `a`?
> > Alternatively, just decompose the whole region into base region and offset 
> > and see if base region has the necessary alignment and the offset is 
> > divisible by the necessary alignment.
> 
> I expect this to be, like, 5 lines of code. I don't understand why the 
> current code is so complicated, it looks like you're considering multiple 
> cases but ultimately doing the same thing.
Thank you for feedback!
Rewroted the code for ElementRegion cases. Now it is much easier to understand, 
and there was also a bug in logic.



Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:261
+
+  if (const VarRegion *TheVarRegion = BaseRegion->getAs()) {
+const VarDecl *TheVarDecl = TheVarRegion->getDecl();

martong wrote:
> martong wrote:
> > Perhaps you could call instead `checkVarRegionAl

[PATCH] D76996: [analyzer] Improve PlacementNewChecker

[Karasev Nikita via Phabricator via cfe-commits]

f00kat created this revision.
f00kat added reviewers: aaron.ballman, lebedev.ri, NoQ, martong.
f00kat added a project: clang.
Herald added subscribers: cfe-commits, ASDenysPetrov, Charusso, dkrupp, 
donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, 
baloghadamsoftware, xazax.hun.

1. Added insufficient storage check for arrays
2. Added align support check

Based on https://reviews.llvm.org/D76229


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76996

Files:
  clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
  clang/test/Analysis/placement-new.cpp

Index: clang/test/Analysis/placement-new.cpp
===
--- clang/test/Analysis/placement-new.cpp
+++ clang/test/Analysis/placement-new.cpp
@@ -155,3 +155,127 @@
   (void)dp;
 }
 } // namespace testHierarchy
+
+namespace testConcretePointers {
+void f1()
+{
+// ok (200 % 8 == 0).
+::new ((size_t*)200) long;
+}
+void f2()
+{
+// bad (100 % 8 == 4).
+::new ((size_t*)100) long; // expected-warning{{Address is aligned to 4 bytes instead of 8 bytes}} expected-note 1 {{}}
+}
+
+void f3()
+{
+::new (reinterpret_cast(200)) long;
+}
+
+void f4()
+{
+::new (reinterpret_cast(100)) long; // expected-warning{{Address is aligned to 4 bytes instead of 8 bytes}} expected-note 1 {{}}
+}
+
+void f5()
+{
+::new ((size_t*)(200 + 0)) long;
+}
+
+void f6() {
+::new ((size_t*)(100 + 2)) long; // expected-warning{{Address is aligned to 6 bytes instead of 8 bytes}} expected-note 1 {{}}
+}
+} // namespace testConcretePointers
+
+namespace testArrayTypesAllocation {
+void f1()
+{
+struct S
+{
+short a;
+};
+
+// bad (not enough space).
+const unsigned N = 32;
+alignas(S) unsigned char buffer1[sizeof(S) * N]; // expected-note {{'buffer1' initialized here}}
+::new (buffer1) S[N]; // expected-warning{{Storage provided to placement new is only 64 bytes, whereas the allocated array type requires more space for internal needs}} expected-note 1 {{}}
+}
+
+void f2() 
+{
+struct S
+{
+short a;
+};
+
+// maybe ok but we need to warn.
+const unsigned N = 32;
+alignas(S) unsigned char buffer2[sizeof(S) * N + sizeof(int)]; // expected-note {{'buffer2' initialized here}}
+::new (buffer2) S[N]; // expected-warning{{Possibly not enough 68 bytes for array allocation which requires 64 bytes. Current overhead requires the size of 4 bytes}} expected-note 1 {{}}
+}
+} // namespace testArrayTypesAllocation
+
+namespace testStructAlign {
+void f1()
+{
+struct A
+{
+char a[9];
+} Ai; // expected-note {{'Ai' initialized here}}
+
+// bad (struct A is aligned to char).
+::new (&Ai.a) long;  // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f2()
+{
+struct B
+{
+char a;
+long b;
+} Bi;
+
+// ok (struct B is aligned to long).
+::new (&Bi.a) long;
+}
+
+void f3()
+{
+struct C
+{
+char a[8];
+alignas(2) char b;
+} Ci; // expected-note {{'Ci' initialized here}}
+
+// bad (struct C is aligned to 2).
+::new (&Ci.a) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f4()
+{
+struct D
+{
+char a;
+char b;
+struct {
+long c;
+};
+} Di;
+
+// ok (struct D is aligned to long).
+::new (&Di.a) long;
+}
+
+void f5()
+{
+struct alignas(alignof(long)) E {
+char a;
+char b;
+} Ei;
+
+// ok (struct E is aligned to long).
+::new (&Ei.a) long;
+}
+} // namespace testStructAlign
+
Index: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
@@ -25,22 +25,31 @@
   void checkPreStmt(const CXXNewExpr *NE, CheckerContext &C) const;
 
 private:
+  bool checkPlaceCapacityIsSufficient(const CXXNewExpr *NE,
+  CheckerContext &C) const;
+
+  bool checkPlaceIsAlignedProperly(const CXXNewExpr *NE,
+   CheckerContext &C) const;
+
   // Returns the size of the target in a placement new expression.
   // E.g. in "new (&s) long" it returns the size of `long`.
-  SVal getExtentSizeOfNewTarget(const CXXNewExpr *NE, ProgramStateRef State,
-CheckerContext &C) const;
+  SVal getExtentSizeOfNewTarget(const CXXNewExpr *NE, CheckerContext &C,
+bool &IsArray) const;
   // Returns the size of the place in a placement new expression.
   // E.g. in "new (&s) long" it returns the size of `s`.
-  SVal getExtentSizeOfPlace(const Expr *NE, ProgramStateRef State,
-Che

[PATCH] D76996: [analyzer] Improve PlacementNewChecker

[Karasev Nikita via Phabricator via cfe-commits]

f00kat updated this revision to Diff 253408.
f00kat added a comment.

lint fixes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76996

Files:
  clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
  clang/test/Analysis/placement-new.cpp

Index: clang/test/Analysis/placement-new.cpp
===
--- clang/test/Analysis/placement-new.cpp
+++ clang/test/Analysis/placement-new.cpp
@@ -155,3 +155,109 @@
   (void)dp;
 }
 } // namespace testHierarchy
+
+namespace testConcretePointers {
+void f1() {
+// ok (200 % 8 == 0).
+::new ((size_t*)200) long;
+}
+void f2() {
+// bad (100 % 8 == 4).
+::new ((size_t*)100) long; // expected-warning{{Address is aligned to 4 bytes instead of 8 bytes}} expected-note 1 {{}}
+}
+
+void f3() {
+::new (reinterpret_cast(200)) long;
+}
+
+void f4() {
+::new (reinterpret_cast(100)) long; // expected-warning{{Address is aligned to 4 bytes instead of 8 bytes}} expected-note 1 {{}}
+}
+
+void f5() {
+::new ((size_t*)(200 + 0)) long;
+}
+
+void f6() {
+::new ((size_t*)(100 + 2)) long; // expected-warning{{Address is aligned to 6 bytes instead of 8 bytes}} expected-note 1 {{}}
+}
+} // namespace testConcretePointers
+
+namespace testArrayTypesAllocation {
+void f1() {
+struct S {
+short a;
+};
+
+// bad (not enough space).
+const unsigned N = 32;
+alignas(S) unsigned char buffer1[sizeof(S) * N]; // expected-note {{'buffer1' initialized here}}
+::new (buffer1) S[N]; // expected-warning{{Storage provided to placement new is only 64 bytes, whereas the allocated array type requires more space for internal needs}} expected-note 1 {{}}
+}
+
+void f2() {
+struct S {
+short a;
+};
+
+// maybe ok but we need to warn.
+const unsigned N = 32;
+alignas(S) unsigned char buffer2[sizeof(S) * N + sizeof(int)]; // expected-note {{'buffer2' initialized here}}
+::new (buffer2) S[N]; // expected-warning{{Possibly not enough 68 bytes for array allocation which requires 64 bytes. Current overhead requires the size of 4 bytes}} expected-note 1 {{}}
+}
+} // namespace testArrayTypesAllocation
+
+namespace testStructAlign {
+void f1() {
+struct A {
+char a[9];
+} Ai; // expected-note {{'Ai' initialized here}}
+
+// bad (struct A is aligned to char).
+::new (&Ai.a) long;  // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f2() {
+struct B {
+char a;
+long b;
+} Bi;
+
+// ok (struct B is aligned to long).
+::new (&Bi.a) long;
+}
+
+void f3() {
+struct C {
+char a[8];
+alignas(2) char b;
+} Ci; // expected-note {{'Ci' initialized here}}
+
+// bad (struct C is aligned to 2).
+::new (&Ci.a) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f4() {
+struct D {
+char a;
+char b;
+struct {
+long c;
+};
+} Di;
+
+// ok (struct D is aligned to long).
+::new (&Di.a) long;
+}
+
+void f5() {
+struct alignas(alignof(long)) E {
+char a;
+char b;
+} Ei;
+
+// ok (struct E is aligned to long).
+::new (&Ei.a) long;
+}
+} // namespace testStructAlign
+
Index: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
@@ -25,22 +25,31 @@
   void checkPreStmt(const CXXNewExpr *NE, CheckerContext &C) const;
 
 private:
+  bool checkPlaceCapacityIsSufficient(const CXXNewExpr *NE,
+  CheckerContext &C) const;
+
+  bool checkPlaceIsAlignedProperly(const CXXNewExpr *NE,
+   CheckerContext &C) const;
+
   // Returns the size of the target in a placement new expression.
   // E.g. in "new (&s) long" it returns the size of `long`.
-  SVal getExtentSizeOfNewTarget(const CXXNewExpr *NE, ProgramStateRef State,
-CheckerContext &C) const;
+  SVal getExtentSizeOfNewTarget(const CXXNewExpr *NE, CheckerContext &C,
+bool &IsArray) const;
   // Returns the size of the place in a placement new expression.
   // E.g. in "new (&s) long" it returns the size of `s`.
-  SVal getExtentSizeOfPlace(const Expr *NE, ProgramStateRef State,
-CheckerContext &C) const;
-  BugType BT{this, "Insufficient storage for placement new",
- categories::MemoryError};
+  SVal getExtentSizeOfPlace(const CXXNewExpr *NE, CheckerContext &C) const;
+  BugType SBT{this, "Insufficient storage for placement new",
+  categories::MemoryError};
+  BugTyp

[PATCH] D76996: [analyzer] Improve PlacementNewChecker

[Karasev Nikita via Phabricator via cfe-commits]

f00kat updated this revision to Diff 253628.
f00kat marked 3 inline comments as done.
f00kat added a comment.

1. Removed code and tests for ConcreteInt cases
2. Fixed FieldRegion check
3. Added handling for ElementRegion cases such as

  void f7() {
short b[10];
  
// ok. 2(short align) + 3*2(index '1' offset)
::new (&b[3]) long;
  }

4. Fixed align error message
5. Maybe fixed lint warnings


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76996

Files:
  clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
  clang/test/Analysis/placement-new.cpp

Index: clang/test/Analysis/placement-new.cpp
===
--- clang/test/Analysis/placement-new.cpp
+++ clang/test/Analysis/placement-new.cpp
@@ -155,3 +155,147 @@
   (void)dp;
 }
 } // namespace testHierarchy
+
+namespace testArrayTypesAllocation {
+void f1() {
+  struct S {
+short a;
+  };
+
+  // bad (not enough space).
+  const unsigned N = 32;
+  alignas(S) unsigned char buffer1[sizeof(S) * N]; // expected-note {{'buffer1' initialized here}}
+  ::new (buffer1) S[N];// expected-warning{{Storage provided to placement new is only 64 bytes, whereas the allocated array type requires more space for internal needs}} expected-note 1 {{}}
+}
+
+void f2() {
+  struct S {
+short a;
+  };
+
+  // maybe ok but we need to warn.
+  const unsigned N = 32;
+  alignas(S) unsigned char buffer2[sizeof(S) * N + sizeof(int)]; // expected-note {{'buffer2' initialized here}}
+  ::new (buffer2) S[N];  // expected-warning{{Possibly not enough 68 bytes for array allocation which requires 64 bytes. Current overhead requires the size of 4 bytes}} expected-note 1 {{}}
+}
+} // namespace testArrayTypesAllocation
+
+namespace testStructAlign {
+void f1() {
+  struct X {
+char a[9];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to char).
+  ::new (&Xi.a) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f2() {
+  struct X {
+char a;
+char b;
+long c;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+
+void f3() {
+  struct X {
+char a;
+char b;
+long c;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to long but field 'b' is aligned to 1 because of its offset)
+  ::new (&Xi.b) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f4() {
+  struct X {
+char a;
+struct alignas(alignof(short)) Y {
+  char b;
+  char c;
+} y;
+long d;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad. 'b' is aligned to short
+  ::new (&Xi.y.b) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f5() {
+  short b[10]; // expected-note {{'b' initialized here}}
+
+  ::new (&b) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f6() {
+  short b[10]; // expected-note {{'b' initialized here}}
+
+  // bad (same as previous but checks ElementRegion case)
+  ::new (&b[0]) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f7() {
+  short b[10];
+
+  // ok. 2(short align) + 3*2(index '1' offset)
+  ::new (&b[3]) long;
+}
+
+void f8() {
+  short b[10]; // expected-note {{'b' initialized here}}
+
+  // bad. 2(short align) + 2*2(index '2' offset)
+  ::new (&b[2]) long; // expected-warning{{Storage type is aligned to 6 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f9() {
+  struct X {
+char a;
+alignas(alignof(short)) char b[20];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // ok 2(custom align) + 6(index '2' offset)
+  ::new (&Xi.b[6]) long;
+
+  // bad 2(custom align) + 1(index '2' offset)
+  ::new (&Xi.b[1]) long; // expected-warning{{Storage type is aligned to 3 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f10() {
+  struct X {
+char a[8];
+alignas(2) char b;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to 2).
+  ::new (&Xi.a) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f11() {
+  struct X {
+char a;
+char b;
+struct Y {
+  long c;
+} d;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+
+void f12() {
+  struct alignas(alignof(long)) X {
+char a;
+char b;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) lon

[PATCH] D76996: [analyzer] Improve PlacementNewChecker

[Karasev Nikita via Phabricator via cfe-commits]

f00kat updated this revision to Diff 253642.
f00kat marked an inline comment as done.
f00kat added a comment.

test fix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76996

Files:
  clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
  clang/test/Analysis/placement-new.cpp

Index: clang/test/Analysis/placement-new.cpp
===
--- clang/test/Analysis/placement-new.cpp
+++ clang/test/Analysis/placement-new.cpp
@@ -155,3 +155,147 @@
   (void)dp;
 }
 } // namespace testHierarchy
+
+namespace testArrayTypesAllocation {
+void f1() {
+  struct S {
+short a;
+  };
+
+  // bad (not enough space).
+  const unsigned N = 32;
+  alignas(S) unsigned char buffer1[sizeof(S) * N]; // expected-note {{'buffer1' initialized here}}
+  ::new (buffer1) S[N];// expected-warning{{Storage provided to placement new is only 64 bytes, whereas the allocated array type requires more space for internal needs}} expected-note 1 {{}}
+}
+
+void f2() {
+  struct S {
+short a;
+  };
+
+  // maybe ok but we need to warn.
+  const unsigned N = 32;
+  alignas(S) unsigned char buffer2[sizeof(S) * N + sizeof(int)]; // expected-note {{'buffer2' initialized here}}
+  ::new (buffer2) S[N];  // expected-warning{{68 bytes is possibly not enough for array allocation which requires 64 bytes. Current overhead requires the size of 4 bytes}} expected-note 1 {{}}
+}
+} // namespace testArrayTypesAllocation
+
+namespace testStructAlign {
+void f1() {
+  struct X {
+char a[9];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to char).
+  ::new (&Xi.a) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f2() {
+  struct X {
+char a;
+char b;
+long c;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+
+void f3() {
+  struct X {
+char a;
+char b;
+long c;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to long but field 'b' is aligned to 1 because of its offset)
+  ::new (&Xi.b) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f4() {
+  struct X {
+char a;
+struct alignas(alignof(short)) Y {
+  char b;
+  char c;
+} y;
+long d;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad. 'b' is aligned to short
+  ::new (&Xi.y.b) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f5() {
+  short b[10]; // expected-note {{'b' initialized here}}
+
+  ::new (&b) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f6() {
+  short b[10]; // expected-note {{'b' initialized here}}
+
+  // bad (same as previous but checks ElementRegion case)
+  ::new (&b[0]) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f7() {
+  short b[10];
+
+  // ok. 2(short align) + 3*2(index '1' offset)
+  ::new (&b[3]) long;
+}
+
+void f8() {
+  short b[10]; // expected-note {{'b' initialized here}}
+
+  // bad. 2(short align) + 2*2(index '2' offset)
+  ::new (&b[2]) long; // expected-warning{{Storage type is aligned to 6 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f9() {
+  struct X {
+char a;
+alignas(alignof(short)) char b[20];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // ok 2(custom align) + 6(index '2' offset)
+  ::new (&Xi.b[6]) long;
+
+  // bad 2(custom align) + 1(index '2' offset)
+  ::new (&Xi.b[1]) long; // expected-warning{{Storage type is aligned to 3 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f10() {
+  struct X {
+char a[8];
+alignas(2) char b;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to 2).
+  ::new (&Xi.a) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f11() {
+  struct X {
+char a;
+char b;
+struct Y {
+  long c;
+} d;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+
+void f12() {
+  struct alignas(alignof(long)) X {
+char a;
+char b;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+} // namespace testStructAlign
Index: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.

[PATCH] D76996: [analyzer] Improve PlacementNewChecker

[Karasev Nikita via Phabricator via cfe-commits]

f00kat updated this revision to Diff 253803.
f00kat marked an inline comment as done.
f00kat added a comment.

Fixed comments in tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76996

Files:
  clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
  clang/test/Analysis/placement-new.cpp

Index: clang/test/Analysis/placement-new.cpp
===
--- clang/test/Analysis/placement-new.cpp
+++ clang/test/Analysis/placement-new.cpp
@@ -155,3 +155,147 @@
   (void)dp;
 }
 } // namespace testHierarchy
+
+namespace testArrayTypesAllocation {
+void f1() {
+  struct S {
+short a;
+  };
+
+  // bad (not enough space).
+  const unsigned N = 32;
+  alignas(S) unsigned char buffer1[sizeof(S) * N]; // expected-note {{'buffer1' initialized here}}
+  ::new (buffer1) S[N];// expected-warning{{Storage provided to placement new is only 64 bytes, whereas the allocated array type requires more space for internal needs}} expected-note 1 {{}}
+}
+
+void f2() {
+  struct S {
+short a;
+  };
+
+  // maybe ok but we need to warn.
+  const unsigned N = 32;
+  alignas(S) unsigned char buffer2[sizeof(S) * N + sizeof(int)]; // expected-note {{'buffer2' initialized here}}
+  ::new (buffer2) S[N];  // expected-warning{{68 bytes is possibly not enough for array allocation which requires 64 bytes. Current overhead requires the size of 4 bytes}} expected-note 1 {{}}
+}
+} // namespace testArrayTypesAllocation
+
+namespace testStructAlign {
+void f1() {
+  struct X {
+char a[9];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to char).
+  ::new (&Xi.a) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f2() {
+  struct X {
+char a;
+char b;
+long c;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+
+void f3() {
+  struct X {
+char a;
+char b;
+long c;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to long but field 'b' is aligned to 1 because of its offset)
+  ::new (&Xi.b) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f4() {
+  struct X {
+char a;
+struct alignas(alignof(short)) Y {
+  char b;
+  char c;
+} y;
+long d;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad. 'b' is aligned to short
+  ::new (&Xi.y.b) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f5() {
+  short b[10]; // expected-note {{'b' initialized here}}
+
+  ::new (&b) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f6() {
+  short b[10]; // expected-note {{'b' initialized here}}
+
+  // bad (same as previous but checks ElementRegion case)
+  ::new (&b[0]) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f7() {
+  short b[10];
+
+  // ok. 2(short align) + 3*2(index '3' offset)
+  ::new (&b[3]) long;
+}
+
+void f8() {
+  short b[10]; // expected-note {{'b' initialized here}}
+
+  // bad. 2(short align) + 2*2(index '2' offset)
+  ::new (&b[2]) long; // expected-warning{{Storage type is aligned to 6 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f9() {
+  struct X {
+char a;
+alignas(alignof(short)) char b[20];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // ok 2(custom align) + 6*1(index '6' offset)
+  ::new (&Xi.b[6]) long;
+
+  // bad 2(custom align) + 1*1(index '1' offset)
+  ::new (&Xi.b[1]) long; // expected-warning{{Storage type is aligned to 3 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f10() {
+  struct X {
+char a[8];
+alignas(2) char b;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to 2).
+  ::new (&Xi.a) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f11() {
+  struct X {
+char a;
+char b;
+struct Y {
+  long c;
+} d;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+
+void f12() {
+  struct alignas(alignof(long)) X {
+char a;
+char b;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+} // namespace testStructAlign
Index: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
+++ clang/lib/StaticAnalyzer/Checkers

[PATCH] D75159: [clang-tidy] Fix PR#37210 'Out-of-class template constructor only half-fixed with modernize-pass-by-value'

[Karasev Nikita via Phabricator via cfe-commits]

f00kat created this revision.
f00kat added reviewers: aaron.ballman, alexfh.
f00kat added projects: clang, clang-tools-extra.
Herald added subscribers: cfe-commits, xazax.hun.

Existing 'modernize-pass-by-value' check works only with non template values in 
initializers.
Now we also skip cases when the value has type 'TemplateSpecializedType' inside 
'cxxConstructExpr'.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75159

Files:
  clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
@@ -118,6 +118,26 @@
 J j1(Movable());
 J j2(NotMovable());
 
+template
+struct MovableTemplateT
+{
+  MovableTemplateT() {}
+  MovableTemplateT(const MovableTemplateT& o) { }
+  MovableTemplateT(MovableTemplateT&& o) { }
+};
+
+template 
+struct J2 {
+  J2(const MovableTemplateT& A);
+  // CHECK-FIXES: J2(const MovableTemplateT& A);
+  MovableTemplateT M;
+};
+
+template 
+J2::J2(const MovableTemplateT& A) : M(A) {}
+// CHECK-FIXES: J2::J2(const MovableTemplateT& A) : M(A) {}
+J2 j3(MovableTemplateT{});
+
 struct K_Movable {
   K_Movable() = default;
   K_Movable(const K_Movable &) = default;
Index: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
@@ -46,8 +46,9 @@
 }
 } // namespace
 
-static TypeMatcher constRefType() {
-  return lValueReferenceType(pointee(isConstQualified()));
+static TypeMatcher notTemplateSpecConstRefType() {
+  return lValueReferenceType(
+  pointee(unless(templateSpecializationType()), isConstQualified()));
 }
 
 static TypeMatcher nonConstValueType() {
@@ -145,16 +146,18 @@
   // ParenListExpr is generated instead of a CXXConstructExpr,
   // filtering out templates automatically for us.
   withInitializer(cxxConstructExpr(
-  has(ignoringParenImpCasts(declRefExpr(to(
-  parmVarDecl(
-  hasType(qualType(
-  // Match only const-ref or a non-const value
-  // parameters. Rvalues and const-values
-  // shouldn't be modified.
-  ValuesOnly ? nonConstValueType()
- : anyOf(constRefType(),
- nonConstValueType()
-  .bind("Param"),
+  has(ignoringParenImpCasts(declRefExpr(
+  to(parmVarDecl(
+ hasType(qualType(
+ // Match only const-ref or a non-const
+ // value parameters. Rvalues,
+ // TemplateSpecializationValues and
+ // const-values shouldn't be modified.
+ ValuesOnly
+ ? nonConstValueType()
+ : anyOf(notTemplateSpecConstRefType(),
+ nonConstValueType()
+ .bind("Param"),
   hasDeclaration(cxxConstructorDecl(
   isCopyConstructor(), unless(isDeleted()),
   hasDeclContext(


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
@@ -118,6 +118,26 @@
 J j1(Movable());
 J j2(NotMovable());
 
+template
+struct MovableTemplateT
+{
+  MovableTemplateT() {}
+  MovableTemplateT(const MovableTemplateT& o) { }
+  MovableTemplateT(MovableTemplateT&& o) { }
+};
+
+template 
+struct J2 {
+  J2(const MovableTemplateT& A);
+  // CHECK-FIXES: J2(const MovableTemplateT& A);
+  MovableTemplateT M;
+};
+
+template 
+J2::J2(const MovableTemplateT& A) : M(A) {}
+// CHECK-FIXES: J2::J2(const MovableTemplateT& A) : M(A) {}
+J2 j3(MovableTemplateT{});
+
 struct K_Movable {
   K_Movable() = default;
   K_Movable(const K_Movable &) = default;
Index: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
+++ 

[PATCH] D75159: [clang-tidy] Fix PR#37210 'Out-of-class template constructor only half-fixed with modernize-pass-by-value'

[Karasev Nikita via Phabricator via cfe-commits]

f00kat added a comment.

Can you commit it please?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75159



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

[PATCH] D76229: [clang-tidy] Added PlacementNewStorageCheck

[Karasev Nikita via Phabricator via cfe-commits]

f00kat created this revision.
f00kat added reviewers: aaron.ballman, alexfh.
f00kat added projects: clang, clang-tools-extra.
Herald added subscribers: cfe-commits, xazax.hun, mgorny.

Added new checker 'cert-mem54-cpp' that checks for CERT rule MEM54-CPP.

I have troubles writing English descriptions for the release notes, so I need 
all the help I can get :)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76229

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/PlacementNewStorageCheck.cpp
  clang-tools-extra/clang-tidy/cert/PlacementNewStorageCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-mem54-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-mem54-cpp.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cert-mem54-cpp.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-mem54-cpp.cpp
@@ -0,0 +1,185 @@
+// RUN: %check_clang_tidy %s cert-mem54-cpp %t
+
+inline void *operator new(size_t s, void *p) noexcept {
+  (void)s;
+  return p;
+}
+
+inline void *operator new[](size_t s, void *p) noexcept {
+  (void)s;
+  return p;
+}
+
+void test1()
+{
+// bad (short is aligned to 2).
+short s1;
+::new (&s1) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 's1' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+
+// ok (same alignment because types are the same).
+long s2;
+::new (&s2) long;
+
+// ok (long long is greater then long).
+long long s3;
+::new (&s3) long;
+
+// bad (alias for type 'short' which is aligned to 2).
+using s4t = short;
+s4t s4;
+::new (&s4) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 's4' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+
+// ok (skip void pointer).
+void* s5 = nullptr;
+::new (s5) long;
+
+// bad (short is aligned to 2).
+short* s6 = nullptr;
+::new (s6) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 's6' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+
+// bad (just check for pointer to pointer).
+short **s7;
+::new (*s7) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 's7' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+}
+
+void test2()
+{
+  // ok.
+  alignas(long) unsigned char buffer1[sizeof(long)];
+  ::new (buffer1) long;
+
+  // bad (buffer is aligned to 'short' instead of 'long').
+  alignas(short) unsigned char buffer2[sizeof(long)];
+  ::new (buffer2) long;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'buffer2' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+
+  // bad (not enough space).
+  alignas(long) unsigned char buffer3[sizeof(short)];
+  ::new (buffer3) long;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 2 bytes are not enough for allocation type 'long' which requires 4 bytes [cert-mem54-cpp]
+
+  // bad (still not enough space).
+  alignas(long) unsigned char buffer4[sizeof(short) + 1];
+  ::new (buffer4) long;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 3 bytes are not enough for allocation type 'long' which requires 4 bytes [cert-mem54-cpp]
+}
+
+void test3()
+{
+// ok (100 % 4 == 0).
+::new ((size_t*)100) long;
+
+// bad (100 % 4 == 2).
+::new ((size_t*)102) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: address is aligned to 2 bytes instead of 4 bytes [cert-mem54-cpp]
+
+::new (reinterpret_cast(100)) long;
+
+::new (reinterpret_cast(102)) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: address is aligned to 2 bytes instead of 4 bytes [cert-mem54-cpp]
+
+::new ((size_t*)(100 + 0)) long;
+
+::new ((size_t*)(100 + 2)) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: address is aligned to 2 bytes instead of 4 bytes [cert-mem54-cpp]
+}
+
+short* test4_f1()
+{
+return nullptr;
+}
+
+short& test4_f2()
+{
+static short v;
+return v;
+}
+
+void test4()
+{
+::new (test4_f1()) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'test4_f1' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+
+::new (&test4_f2()) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'test4_f2' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+
+short* (*p1)();
+p1 = test4_f1;
+::new (p1()) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'p1' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+}
+
+void test5()
+{
+  struct S
+  {
+  short a;
+  };
+
+  // bad (not enough space).
+  const unsigned N = 32;
+  alignas(S) unsigned char bu

[PATCH] D76229: [clang-tidy] Added PlacementNewStorageCheck

[Karasev Nikita via Phabricator via cfe-commits]

f00kat updated this revision to Diff 250556.
f00kat added a comment.

Fix 'const auto*' in function getDescendantValueDecl


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76229

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/PlacementNewStorageCheck.cpp
  clang-tools-extra/clang-tidy/cert/PlacementNewStorageCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-mem54-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-mem54-cpp.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cert-mem54-cpp.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-mem54-cpp.cpp
@@ -0,0 +1,185 @@
+// RUN: %check_clang_tidy %s cert-mem54-cpp %t
+
+inline void *operator new(size_t s, void *p) noexcept {
+  (void)s;
+  return p;
+}
+
+inline void *operator new[](size_t s, void *p) noexcept {
+  (void)s;
+  return p;
+}
+
+void test1()
+{
+// bad (short is aligned to 2).
+short s1;
+::new (&s1) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 's1' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+
+// ok (same alignment because types are the same).
+long s2;
+::new (&s2) long;
+
+// ok (long long is greater then long).
+long long s3;
+::new (&s3) long;
+
+// bad (alias for type 'short' which is aligned to 2).
+using s4t = short;
+s4t s4;
+::new (&s4) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 's4' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+
+// ok (skip void pointer).
+void* s5 = nullptr;
+::new (s5) long;
+
+// bad (short is aligned to 2).
+short* s6 = nullptr;
+::new (s6) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 's6' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+
+// bad (just check for pointer to pointer).
+short **s7;
+::new (*s7) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 's7' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+}
+
+void test2()
+{
+  // ok.
+  alignas(long) unsigned char buffer1[sizeof(long)];
+  ::new (buffer1) long;
+
+  // bad (buffer is aligned to 'short' instead of 'long').
+  alignas(short) unsigned char buffer2[sizeof(long)];
+  ::new (buffer2) long;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'buffer2' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+
+  // bad (not enough space).
+  alignas(long) unsigned char buffer3[sizeof(short)];
+  ::new (buffer3) long;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 2 bytes are not enough for allocation type 'long' which requires 4 bytes [cert-mem54-cpp]
+
+  // bad (still not enough space).
+  alignas(long) unsigned char buffer4[sizeof(short) + 1];
+  ::new (buffer4) long;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 3 bytes are not enough for allocation type 'long' which requires 4 bytes [cert-mem54-cpp]
+}
+
+void test3()
+{
+// ok (100 % 4 == 0).
+::new ((size_t*)100) long;
+
+// bad (100 % 4 == 2).
+::new ((size_t*)102) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: address is aligned to 2 bytes instead of 4 bytes [cert-mem54-cpp]
+
+::new (reinterpret_cast(100)) long;
+
+::new (reinterpret_cast(102)) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: address is aligned to 2 bytes instead of 4 bytes [cert-mem54-cpp]
+
+::new ((size_t*)(100 + 0)) long;
+
+::new ((size_t*)(100 + 2)) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: address is aligned to 2 bytes instead of 4 bytes [cert-mem54-cpp]
+}
+
+short* test4_f1()
+{
+return nullptr;
+}
+
+short& test4_f2()
+{
+static short v;
+return v;
+}
+
+void test4()
+{
+::new (test4_f1()) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'test4_f1' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+
+::new (&test4_f2()) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'test4_f2' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+
+short* (*p1)();
+p1 = test4_f1;
+::new (p1()) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'p1' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+}
+
+void test5()
+{
+  struct S
+  {
+  short a;
+  };
+
+  // bad (not enough space).
+  const unsigned N = 32;
+  alignas(S) unsigned char buffer1[sizeof(S) * N];
+  ::new (buffer1) S[N];
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 64 bytes are not enough for array allocation which requires 64 bytes [cert-mem54

[PATCH] D76229: [clang-tidy] Added PlacementNewStorageCheck

[Karasev Nikita via Phabricator via cfe-commits]

f00kat updated this revision to Diff 250555.
f00kat added a comment.

1. Static functions instead of anonymous namespaces.
2. const auto*.
3. Added double back-ticks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76229

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/PlacementNewStorageCheck.cpp
  clang-tools-extra/clang-tidy/cert/PlacementNewStorageCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-mem54-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-mem54-cpp.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cert-mem54-cpp.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-mem54-cpp.cpp
@@ -0,0 +1,185 @@
+// RUN: %check_clang_tidy %s cert-mem54-cpp %t
+
+inline void *operator new(size_t s, void *p) noexcept {
+  (void)s;
+  return p;
+}
+
+inline void *operator new[](size_t s, void *p) noexcept {
+  (void)s;
+  return p;
+}
+
+void test1()
+{
+// bad (short is aligned to 2).
+short s1;
+::new (&s1) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 's1' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+
+// ok (same alignment because types are the same).
+long s2;
+::new (&s2) long;
+
+// ok (long long is greater then long).
+long long s3;
+::new (&s3) long;
+
+// bad (alias for type 'short' which is aligned to 2).
+using s4t = short;
+s4t s4;
+::new (&s4) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 's4' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+
+// ok (skip void pointer).
+void* s5 = nullptr;
+::new (s5) long;
+
+// bad (short is aligned to 2).
+short* s6 = nullptr;
+::new (s6) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 's6' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+
+// bad (just check for pointer to pointer).
+short **s7;
+::new (*s7) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 's7' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+}
+
+void test2()
+{
+  // ok.
+  alignas(long) unsigned char buffer1[sizeof(long)];
+  ::new (buffer1) long;
+
+  // bad (buffer is aligned to 'short' instead of 'long').
+  alignas(short) unsigned char buffer2[sizeof(long)];
+  ::new (buffer2) long;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'buffer2' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+
+  // bad (not enough space).
+  alignas(long) unsigned char buffer3[sizeof(short)];
+  ::new (buffer3) long;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 2 bytes are not enough for allocation type 'long' which requires 4 bytes [cert-mem54-cpp]
+
+  // bad (still not enough space).
+  alignas(long) unsigned char buffer4[sizeof(short) + 1];
+  ::new (buffer4) long;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 3 bytes are not enough for allocation type 'long' which requires 4 bytes [cert-mem54-cpp]
+}
+
+void test3()
+{
+// ok (100 % 4 == 0).
+::new ((size_t*)100) long;
+
+// bad (100 % 4 == 2).
+::new ((size_t*)102) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: address is aligned to 2 bytes instead of 4 bytes [cert-mem54-cpp]
+
+::new (reinterpret_cast(100)) long;
+
+::new (reinterpret_cast(102)) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: address is aligned to 2 bytes instead of 4 bytes [cert-mem54-cpp]
+
+::new ((size_t*)(100 + 0)) long;
+
+::new ((size_t*)(100 + 2)) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: address is aligned to 2 bytes instead of 4 bytes [cert-mem54-cpp]
+}
+
+short* test4_f1()
+{
+return nullptr;
+}
+
+short& test4_f2()
+{
+static short v;
+return v;
+}
+
+void test4()
+{
+::new (test4_f1()) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'test4_f1' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+
+::new (&test4_f2()) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'test4_f2' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+
+short* (*p1)();
+p1 = test4_f1;
+::new (p1()) long;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'p1' is aligned to 2 bytes but allocated type 'long' is aligned to 4 bytes [cert-mem54-cpp]
+}
+
+void test5()
+{
+  struct S
+  {
+  short a;
+  };
+
+  // bad (not enough space).
+  const unsigned N = 32;
+  alignas(S) unsigned char buffer1[sizeof(S) * N];
+  ::new (buffer1) S[N];
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 64 bytes are not enough for array al

[PATCH] D76229: [clang-tidy] Added PlacementNewStorageCheck

[Karasev Nikita via Phabricator via cfe-commits]

f00kat added a comment.

In D76229#1925268 , @aaron.ballman 
wrote:

> Have you considered making this a static analyzer check as opposed to a 
> clang-tidy check? I think using dataflow analysis will really reduce the 
> false positives because it will be able to track the allocation and alignment 
> data across control flow.


I have never try to write static analyzer before. Okay, I will look at examples 
of how to work with dataflow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76229



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

[PATCH] D76229: [clang-tidy] Added PlacementNewStorageCheck

[Karasev Nikita via Phabricator via cfe-commits]

f00kat added a comment.

In D76229#1925360 , @lebedev.ri wrote:

> This seems to be already handled by clang static analyzer? 
> (`clang-analyzer-cplusplus.PlacementNew`)
>
>   :19:5: warning: Storage provided to placement new is only 2 bytes, 
> whereas the allocated type requires 8 bytes 
> [clang-analyzer-cplusplus.PlacementNew]
>   ::new (&s1) long;
>   ^
>   :19:5: note: Storage provided to placement new is only 2 bytes, 
> whereas the allocated type requires 8 bytes
>   :64:3: warning: Storage provided to placement new is only 2 bytes, 
> whereas the allocated type requires 8 bytes 
> [clang-analyzer-cplusplus.PlacementNew]
> ::new (buffer3) long;
> ^
>
>
> https://godbolt.org/z/9VX5WW


But it seems like not all of my tests pass on static analyzer?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76229



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

[PATCH] D76229: [clang-tidy] Added PlacementNewStorageCheck

[Karasev Nikita via Phabricator via cfe-commits]

f00kat added a comment.

In D76229#1925360 , @lebedev.ri wrote:

> This seems to be already handled by clang static analyzer? 
> (`clang-analyzer-cplusplus.PlacementNew`)
>
>   :19:5: warning: Storage provided to placement new is only 2 bytes, 
> whereas the allocated type requires 8 bytes 
> [clang-analyzer-cplusplus.PlacementNew]
>   ::new (&s1) long;
>   ^
>   :19:5: note: Storage provided to placement new is only 2 bytes, 
> whereas the allocated type requires 8 bytes
>   :64:3: warning: Storage provided to placement new is only 2 bytes, 
> whereas the allocated type requires 8 bytes 
> [clang-analyzer-cplusplus.PlacementNew]
> ::new (buffer3) long;
> ^
>
>
> https://godbolt.org/z/9VX5WW


Oh no..It is sad :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76229



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

[PATCH] D76229: [clang-tidy] Added PlacementNewStorageCheck

[Karasev Nikita via Phabricator via cfe-commits]

f00kat added a comment.

In D76229#1925948 , @lebedev.ri wrote:

> In D76229#1925371 , @f00kat wrote:
>
> > In D76229#1925360 , @lebedev.ri 
> > wrote:
> >
> > > This seems to be already handled by clang static analyzer? 
> > > (`clang-analyzer-cplusplus.PlacementNew`)
> > >
> > >   :19:5: warning: Storage provided to placement new is only 2 
> > > bytes, whereas the allocated type requires 8 bytes 
> > > [clang-analyzer-cplusplus.PlacementNew]
> > >   ::new (&s1) long;
> > >   ^
> > >   :19:5: note: Storage provided to placement new is only 2 bytes, 
> > > whereas the allocated type requires 8 bytes
> > >   :64:3: warning: Storage provided to placement new is only 2 
> > > bytes, whereas the allocated type requires 8 bytes 
> > > [clang-analyzer-cplusplus.PlacementNew]
> > > ::new (buffer3) long;
> > > ^
> > >
> > >
> > > https://godbolt.org/z/9VX5WW
> >
> >
> > But it seems like not all of my tests pass on static analyzer?
>
>
> I have not really worked with static analyzer code, but assuming that those 
> cases
>  that are no longer diagnosed as compared to this clang-tidy checks *should* 
> be diagnosed
>  (i.e. diagnosing them isn't false-positive), then i'd think that static 
> analyzer check
>  simply needs some work?


Yeah. You are right. I will try to improve exist checker.
What we gonna do with this patch? Remove it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76229



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

[PATCH] D76229: [clang-tidy] Added PlacementNewStorageCheck

[Karasev Nikita via Phabricator via cfe-commits]

f00kat added a comment.

NoQ, I don`t want to say that existing static alalyzer is bad written or 
something. I just noticed that it does not yet support arrays and alignment.
Anyway thank you for your detailed feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76229



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

[PATCH] D76229: [clang-tidy] Added PlacementNewStorageCheck

[Karasev Nikita via Phabricator via cfe-commits]

f00kat added a comment.

In D76229#1926818 , @Charusso wrote:

> In D76229#1925363 , @f00kat wrote:
>
> > In D76229#1925268 , @aaron.ballman 
> > wrote:
> >
> > > Have you considered making this a static analyzer check as opposed to a 
> > > clang-tidy check? I think using dataflow analysis will really reduce the 
> > > false positives because it will be able to track the allocation and 
> > > alignment data across control flow.
> >
> >
> > I have never try to write static analyzer before. Okay, I will look at 
> > examples of how to work with dataflow.
>
>
> To getting started with the Analyzer please visit @NoQ's (tiny bit outdated) 
> booklet: https://github.com/haoNoQ/clang-analyzer-guide
>
> Advertisement:
>  If D69726  lands we will have an arsenal of 
> APIs for helping dynamic size based checkers.
>  If D73521  lands it should be easier to 
> getting started with the Analyzer, but now you could visit what is our 
> current API.


Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76229



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

[PATCH] D73090: [clang-tidy] Fix PR#44528 'modernize-use-using and enums'

[Karasev Nikita via Phabricator via cfe-commits]

f00kat updated this revision to Diff 241931.
f00kat added a comment.

1. clang-format
2. Add comment for additional "files IDs equals" check
3. Add test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73090

Files:
  clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-use-using/modernize-use-using.h
  clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s modernize-use-using %t
+// RUN: %check_clang_tidy %s modernize-use-using %t -- -- -I %S/Inputs/modernize-use-using/
 
 typedef int Type;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef' [modernize-use-using]
@@ -267,3 +267,14 @@
 // CHECK-MESSAGES: :[[@LINE-2]]:30: warning: use 'using' instead of 'typedef'
 // CHECK-FIXES: using R_t = struct { int a; };
 // CHECK-FIXES-NEXT: using R_p = R_t*;
+
+typedef enum { ea1, eb1 } EnumT1;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using EnumT1 = enum { ea1, eb1 };
+
+#include "modernize-use-using.h"
+
+typedef enum { ea2, eb2 } EnumT2_CheckTypedefImpactFromAnotherFile;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using EnumT2_CheckTypedefImpactFromAnotherFile = enum { ea2, eb2 };
+
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-use-using/modernize-use-using.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-use-using/modernize-use-using.h
@@ -0,0 +1,6 @@
+#ifndef MODERNIZE_USE_USING_H
+#define MODERNIZE_USE_USING_H
+
+typedef int mytype;
+
+#endif
Index: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
===
--- clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
+++ clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
@@ -23,7 +23,7 @@
 
   const bool IgnoreMacros;
   SourceLocation LastReplacementEnd;
-  SourceRange LastCxxDeclRange;
+  SourceRange LastTagDeclRange;
   std::string FirstTypedefType;
   std::string FirstTypedefName;
 
Index: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
@@ -25,18 +25,17 @@
 return;
   Finder->addMatcher(typedefDecl(unless(isInstantiated())).bind("typedef"),
  this);
-  // This matcher used to find structs defined in source code within typedefs.
+  // This matcher used to find tag declarations in source code within typedefs.
   // They appear in the AST just *prior* to the typedefs.
-  Finder->addMatcher(cxxRecordDecl(unless(isImplicit())).bind("struct"), this);
+  Finder->addMatcher(tagDecl(unless(isImplicit())).bind("tagdecl"), this);
 }
 
 void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
   // Match CXXRecordDecl only to store the range of the last non-implicit full
   // declaration, to later check whether it's within the typdef itself.
-  const auto *MatchedCxxRecordDecl =
-  Result.Nodes.getNodeAs("struct");
-  if (MatchedCxxRecordDecl) {
-LastCxxDeclRange = MatchedCxxRecordDecl->getSourceRange();
+  const auto *MatchedTagDecl = Result.Nodes.getNodeAs("tagdecl");
+  if (MatchedTagDecl) {
+LastTagDeclRange = MatchedTagDecl->getSourceRange();
 return;
   }
 
@@ -70,9 +69,13 @@
   // consecutive TypedefDecl nodes whose SourceRanges overlap. Each range starts
   // at the "typedef" and then continues *across* previous definitions through
   // the end of the current TypedefDecl definition.
+  // But also we need to check that ranges belongs the same file because
+  // different files may contain overlap ranges.
   std::string Using = "using ";
   if (ReplaceRange.getBegin().isMacroID() ||
-  ReplaceRange.getBegin() >= LastReplacementEnd) {
+  (Result.SourceManager->getFileID(ReplaceRange.getBegin()) !=
+   Result.SourceManager->getFileID(LastReplacementEnd)) ||
+  (ReplaceRange.getBegin() >= LastReplacementEnd)) {
 // This is the first (and possibly the only) TypedefDecl in a typedef. Save
 // Type and Name in case we find subsequent TypedefDecl's in this typedef.
 FirstTypedefType = Type;
@@ -95,11 +98,12 @@
 
   auto Diag = diag(ReplaceRange.getBegin(), UseUsingWarning);
 
-  // If typedef contains a full struct/class declaration, extract 

[PATCH] D73090: [clang-tidy] Fix PR#44528 'modernize-use-using and enums'

[Karasev Nikita via Phabricator via cfe-commits]

f00kat updated this revision to Diff 241971.
f00kat marked an inline comment as done.
f00kat added a comment.

Clean up


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73090

Files:
  clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-use-using/modernize-use-using.h
  clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s modernize-use-using %t
+// RUN: %check_clang_tidy %s modernize-use-using %t -- -- -I %S/Inputs/modernize-use-using/
 
 typedef int Type;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef' [modernize-use-using]
@@ -267,3 +267,14 @@
 // CHECK-MESSAGES: :[[@LINE-2]]:30: warning: use 'using' instead of 'typedef'
 // CHECK-FIXES: using R_t = struct { int a; };
 // CHECK-FIXES-NEXT: using R_p = R_t*;
+
+typedef enum { ea1, eb1 } EnumT1;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using EnumT1 = enum { ea1, eb1 };
+
+#include "modernize-use-using.h"
+
+typedef enum { ea2, eb2 } EnumT2_CheckTypedefImpactFromAnotherFile;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using EnumT2_CheckTypedefImpactFromAnotherFile = enum { ea2, eb2 };
+
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-use-using/modernize-use-using.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-use-using/modernize-use-using.h
@@ -0,0 +1,6 @@
+#ifndef MODERNIZE_USE_USING_H
+#define MODERNIZE_USE_USING_H
+
+typedef int mytype;
+
+#endif
Index: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
===
--- clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
+++ clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
@@ -23,7 +23,7 @@
 
   const bool IgnoreMacros;
   SourceLocation LastReplacementEnd;
-  SourceRange LastCxxDeclRange;
+  SourceRange LastTagDeclRange;
   std::string FirstTypedefType;
   std::string FirstTypedefName;
 
Index: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
@@ -25,18 +25,17 @@
 return;
   Finder->addMatcher(typedefDecl(unless(isInstantiated())).bind("typedef"),
  this);
-  // This matcher used to find structs defined in source code within typedefs.
+  // This matcher used to find tag declarations in source code within typedefs.
   // They appear in the AST just *prior* to the typedefs.
-  Finder->addMatcher(cxxRecordDecl(unless(isImplicit())).bind("struct"), this);
+  Finder->addMatcher(tagDecl(unless(isImplicit())).bind("tagdecl"), this);
 }
 
 void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
   // Match CXXRecordDecl only to store the range of the last non-implicit full
   // declaration, to later check whether it's within the typdef itself.
-  const auto *MatchedCxxRecordDecl =
-  Result.Nodes.getNodeAs("struct");
-  if (MatchedCxxRecordDecl) {
-LastCxxDeclRange = MatchedCxxRecordDecl->getSourceRange();
+  const auto *MatchedTagDecl = Result.Nodes.getNodeAs("tagdecl");
+  if (MatchedTagDecl) {
+LastTagDeclRange = MatchedTagDecl->getSourceRange();
 return;
   }
 
@@ -70,9 +69,13 @@
   // consecutive TypedefDecl nodes whose SourceRanges overlap. Each range starts
   // at the "typedef" and then continues *across* previous definitions through
   // the end of the current TypedefDecl definition.
+  // But also we need to check that the ranges belong to the same file because
+  // different files may contain overlapping ranges.
   std::string Using = "using ";
   if (ReplaceRange.getBegin().isMacroID() ||
-  ReplaceRange.getBegin() >= LastReplacementEnd) {
+  (Result.SourceManager->getFileID(ReplaceRange.getBegin()) !=
+   Result.SourceManager->getFileID(LastReplacementEnd)) ||
+  (ReplaceRange.getBegin() >= LastReplacementEnd)) {
 // This is the first (and possibly the only) TypedefDecl in a typedef. Save
 // Type and Name in case we find subsequent TypedefDecl's in this typedef.
 FirstTypedefType = Type;
@@ -95,11 +98,12 @@
 
   auto Diag = diag(ReplaceRange.getBegin(), UseUsingWarning);
 
-  // If typedef contains a full struct/class declaration, extract its full text.
-  if (L

[PATCH] D73090: [clang-tidy] Fix PR#44528 'modernize-use-using and enums'

[Karasev Nikita via Phabricator via cfe-commits]

f00kat added a comment.

Sorry for my english. Sometimes it's more difficult than bug fixing




Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp:74
   if (ReplaceRange.getBegin().isMacroID() ||
-  ReplaceRange.getBegin() >= LastReplacementEnd) {
+  (Result.SourceManager->getFileID(ReplaceRange.getBegin()) != 
Result.SourceManager->getFileID(LastReplacementEnd)) ||
+  (ReplaceRange.getBegin() >= LastReplacementEnd)) {

aaron.ballman wrote:
> Be sure to run clang-format over the patch; this looks well beyond the usual 
> 80-col limit. You can also drop the unnecessary parens around the sub 
> expressions.
> 
> Also, a comment explaining why this code is needed would help future folks as 
> would a test case showing what this corrects.
I left the brackets because they seem to increase readability. Or not? :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73090



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

[PATCH] D74033: [clang-tidy] Fix PR#44620 'readability-redundant-string-cstr quick-fix causes invalid code'

[Karasev Nikita via Phabricator via cfe-commits]

f00kat created this revision.
f00kat added reviewers: aaron.ballman, alexfh.
f00kat added projects: clang-tools-extra, clang.
Herald added subscribers: cfe-commits, xazax.hun.

  #include 
  
  static void f2(std::string&&) {
  }
  
  static void f() {
std::string const s;
f2(s.c_str()); // readability-redundant-string-cstr warning
  }

For this case I decide to do nothing by skipping it in the matcher. Another way 
is to fix with 'std::move(s)' but I`m not sure that it is correct.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74033

Files:
  clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-cstr.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-cstr.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-cstr.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-cstr.cpp
@@ -205,3 +205,9 @@
 void invalid(const NotAString &s) {
   dummy(s.c_str());
 }
+
+// Test for rvalue std::string
+
+void m1(std::string&& s) {
+  m1(s.c_str());
+}
Index: clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
@@ -61,6 +61,54 @@
   return (llvm::Twine("*") + Text).str();
 }
 
+// Trying to get CallExpr in which CxxConstructExpr is called
+const clang::CallExpr *
+tryGetCallExprAncestorForCxxConstructExpr(const Expr *TheExpr,
+ ASTContext &Context) {
+  // We skip nodes such as CXXBindTemporaryExpr, MaterializeTemporaryExpr
+  for (ast_type_traits::DynTypedNode DynParent : Context.getParents(*TheExpr)) 
{
+if (const auto *Parent = DynParent.get()) {
+  if (const auto *TheCallExpr = dyn_cast(Parent))
+return TheCallExpr;
+
+  if (const clang::CallExpr *TheCallExpr =
+  tryGetCallExprAncestorForCxxConstructExpr(Parent, Context))
+return TheCallExpr;
+}
+  }
+
+  return nullptr;
+}
+
+// Check that ParamDecl of CallExprDecl has rvalue type
+bool checkParamDeclOfAncestorCallExprHasRValueRefType(
+const Expr *TheCxxConstructExpr, ASTContext &Context) {
+  if (const clang::CallExpr *TheCallExpr =
+  tryGetCallExprAncestorForCxxConstructExpr(TheCxxConstructExpr,
+   Context)) {
+for (int i = 0; i < TheCallExpr->getNumArgs(); ++i) {
+  const Expr *Arg = TheCallExpr->getArg(i);
+  if (Arg->getSourceRange() == TheCxxConstructExpr->getSourceRange()) {
+if (const auto *TheCallExprDecl =
+dyn_cast(TheCallExpr->getCalleeDecl())) {
+  if (TheCallExprDecl->getParamDecl(i)
+  ->getType()
+  ->isRValueReferenceType())
+return true;
+}
+  }
+}
+  }
+
+  return false;
+}
+
+AST_MATCHER(CXXConstructExpr,
+matchedParamDeclOfAncestorCallExprHasRValueRefType) {
+  return checkParamDeclOfAncestorCallExprHasRValueRefType(
+  &Node, Finder->getASTContext());
+}
+
 } // end namespace
 
 void RedundantStringCStrCheck::registerMatchers(
@@ -95,9 +143,13 @@
   .bind("call");
 
   // Detect redundant 'c_str()' calls through a string constructor.
-  Finder->addMatcher(cxxConstructExpr(StringConstructorExpr,
-  hasArgument(0, StringCStrCallExpr)),
- this);
+  // If CxxConstructExpr is the part of some CallExpr we need to
+  // check that matched ParamDecl of the ancestor CallExpr is not rvalue
+  Finder->addMatcher(
+  cxxConstructExpr(
+  StringConstructorExpr, hasArgument(0, StringCStrCallExpr),
+  unless(matchedParamDeclOfAncestorCallExprHasRValueRefType())),
+  this);
 
   // Detect: 's == str.c_str()'  ->  's == str'
   Finder->addMatcher(


Index: clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-cstr.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-cstr.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-cstr.cpp
@@ -205,3 +205,9 @@
 void invalid(const NotAString &s) {
   dummy(s.c_str());
 }
+
+// Test for rvalue std::string
+
+void m1(std::string&& s) {
+  m1(s.c_str());
+}
Index: clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
@@ -61,6 +61,54 @@
   return (llvm::Twine("*") + Text).str();
 }
 
+// Trying to get C

[PATCH] D74214: [clang-tidy] Fix PR#34798 'readability-braces-around-statements breaks statements containing braces.'

[Karasev Nikita via Phabricator via cfe-commits]

f00kat created this revision.
f00kat added reviewers: aaron.ballman, alexfh.
f00kat added projects: clang, clang-tools-extra.
Herald added subscribers: cfe-commits, xazax.hun.

In method 'BracesAroundStatementsCheck::checkStmt' the call 
'Lexer::makeFileCharRange(line 253)' return different EndSourceLocation for 
different statements.

  CharSourceRange FileRange = Lexer::makeFileCharRange(
CharSourceRange::getTokenRange(S->getSourceRange()), SM,
Context->getLangOpts());

For example

  class c {};
  c test()
  {
if (true)
return {};
if (true)
bool b[2] = { true, true };
  }

In case

  return {};

FileRange.getEnd() will be ';'
In case

  bool b[2] = { true, true };

FileRange.getEnd() will be '\r'

Before call 'findEndLocation' we do this

  const auto FREnd = FileRange.getEnd().getLocWithOffset(-1);

so 'findEndLocation' received '}' in the first case and ';' in the second what 
breaks the logic of 'findEndLocation'.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74214

Files:
  clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp
@@ -204,3 +204,28 @@
   // CHECK-FIXES-NEXT: {{^  ;$}}
   // CHECK-FIXES-NEXT: {{^}$}}
 }
+
+class X {};
+X test_initListExpr()
+{
+  if (cond("x")) return {};
+  else return {};
+  // CHECK-MESSAGES: :[[@LINE-2]]:24: warning: statement should be inside 
braces
+  // CHECK-FIXES: if (cond("x")) { return {};
+  // CHECK-FIXES: } else {
+  // CHECK-FIXES: return {};
+  // CHECK-FIXES: }
+
+  if (cond("y1"))
+if (cond("y2") && cond("y3"))
+  return {};
+else
+  return {};
+  // CHECK-MESSAGES: :[[@LINE-2]]:24: warning: statement should be inside 
braces
+  // CHECK-FIXES: if (cond("y1")) {
+  // CHECK-FIXES: if (cond("y2") && cond("y3")) {
+  // CHECK-FIXES: return {};
+  // CHECK-FIXES: } else {
+  // CHECK-FIXES: return {};
+  // CHECK-FIXES: }
+}
Index: clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp
@@ -18,19 +18,32 @@
 namespace readability {
 namespace {
 
-tok::TokenKind getTokenKind(SourceLocation Loc, const SourceManager &SM,
-const ASTContext *Context) {
-  Token Tok;
+bool tryGetTokenKind(SourceLocation Loc, const SourceManager &SM,
+ const ASTContext *Context, tok::TokenKind &Kind) {
+  Kind = tok::NUM_TOKENS;
   SourceLocation Beginning =
   Lexer::GetBeginningOfToken(Loc, SM, Context->getLangOpts());
+
+  Token Tok;
   const bool Invalid =
   Lexer::getRawToken(Beginning, Tok, SM, Context->getLangOpts());
-  assert(!Invalid && "Expected a valid token.");
+  if (!Invalid) {
+Kind = Tok.getKind();
+  }
+
+  return !Invalid;
+}
+
+tok::TokenKind getTokenKind(SourceLocation Loc, const SourceManager &SM,
+const ASTContext *Context) {
+  tok::TokenKind Kind;
+  if (tryGetTokenKind(Loc, SM, Context, Kind)) {
+return Kind;
+  }
 
-  if (Invalid)
-return tok::NUM_TOKENS;
+  assert(false && "Expected a valid token.");
 
-  return Tok.getKind();
+  return tok::NUM_TOKENS;
 }
 
 SourceLocation forwardSkipWhitespaceAndComments(SourceLocation Loc,
@@ -58,8 +71,21 @@
   // Loc points to the beginning of the last (non-comment non-ws) token
   // before end or ';'.
   assert(Loc.isValid());
-  bool SkipEndWhitespaceAndComments = true;
   tok::TokenKind TokKind = getTokenKind(Loc, SM, Context);
+
+  // We need to check that it is not 'InitListExpr' which ends with 
+  // the tokens '};' because it will break the following analysis
+  tok::TokenKind NextTokKind;
+  SourceLocation NextLoc =
+  Lexer::getLocForEndOfToken(Loc, 0, SM, Context->getLangOpts());
+  if (tryGetTokenKind(NextLoc, SM, Context, NextTokKind)) {
+if (TokKind == tok::r_brace && NextTokKind == tok::semi) {
+  Loc = NextLoc;
+  TokKind = NextTokKind;
+}
+  }
+
+  bool SkipEndWhitespaceAndComments = true;
   if (TokKind == tok::NUM_TOKENS || TokKind == tok::semi ||
   TokKind == tok::r_brace) {
 // If we are at ";" or "}", we found the last token. We could use as well


Index: clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp
++

[PATCH] D74033: [clang-tidy] Fix PR#44620 'readability-redundant-string-cstr quick-fix causes invalid code'

[Karasev Nikita via Phabricator via cfe-commits]

f00kat updated this revision to Diff 243833.
f00kat added a comment.

1. Add full stops at the end of the lines.
2. Add some static.
3. Fix function 'checkParamDeclOfAncestorCallExprHasRValueRefType' to work well 
with the function pointer calls.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74033

Files:
  clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-cstr.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-cstr.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-cstr.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-cstr.cpp
@@ -205,3 +205,18 @@
 void invalid(const NotAString &s) {
   dummy(s.c_str());
 }
+
+// Test for rvalue std::string.
+void m1(std::string&&) {
+  std::string s;
+
+  m1(s.c_str());
+
+  void (*m1p1)(std::string&&);
+  m1p1 = m1;
+  m1p1(s.c_str());
+
+  using m1tp = void (*)(std::string &&);
+  m1tp m1p2 = m1;
+  m1p2(s.c_str());  
+}
Index: clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
@@ -61,6 +61,55 @@
   return (llvm::Twine("*") + Text).str();
 }
 
+// Trying to get CallExpr in which CxxConstructExpr is called.
+static const clang::CallExpr *
+tryGetCallExprAncestorForCxxConstructExpr(const Expr *TheExpr,
+  ASTContext &Context) {
+  // We skip nodes such as CXXBindTemporaryExpr, MaterializeTemporaryExpr.
+  for (ast_type_traits::DynTypedNode DynParent : Context.getParents(*TheExpr)) 
{
+if (const auto *Parent = DynParent.get()) {
+  if (const auto *TheCallExpr = dyn_cast(Parent))
+return TheCallExpr;
+
+  if (const clang::CallExpr *TheCallExpr =
+  tryGetCallExprAncestorForCxxConstructExpr(Parent, Context))
+return TheCallExpr;
+}
+  }
+
+  return nullptr;
+}
+
+// Check that ParamDecl of CallExprDecl has rvalue type.
+static bool checkParamDeclOfAncestorCallExprHasRValueRefType(
+const Expr *TheCxxConstructExpr, ASTContext &Context) {
+  if (const clang::CallExpr *TheCallExpr =
+  tryGetCallExprAncestorForCxxConstructExpr(TheCxxConstructExpr,
+Context)) {
+for (int i = 0; i < TheCallExpr->getNumArgs(); ++i) {
+  const Expr *Arg = TheCallExpr->getArg(i);
+  if (Arg->getSourceRange() == TheCxxConstructExpr->getSourceRange()) {
+if (const auto *TheCallExprFuncProto =
+TheCallExpr->getCallee()
+->getType()
+->getPointeeType()
+->getAs()) {
+  if (TheCallExprFuncProto->getParamType(i)->isRValueReferenceType())
+return true;
+}
+  }
+}
+  }
+
+  return false;
+}
+
+AST_MATCHER(CXXConstructExpr,
+matchedParamDeclOfAncestorCallExprHasRValueRefType) {
+  return checkParamDeclOfAncestorCallExprHasRValueRefType(
+  &Node, Finder->getASTContext());
+}
+
 } // end namespace
 
 void RedundantStringCStrCheck::registerMatchers(
@@ -95,9 +144,13 @@
   .bind("call");
 
   // Detect redundant 'c_str()' calls through a string constructor.
-  Finder->addMatcher(cxxConstructExpr(StringConstructorExpr,
-  hasArgument(0, StringCStrCallExpr)),
- this);
+  // If CxxConstructExpr is the part of some CallExpr we need to
+  // check that matched ParamDecl of the ancestor CallExpr is not rvalue.
+  Finder->addMatcher(
+  cxxConstructExpr(
+  StringConstructorExpr, hasArgument(0, StringCStrCallExpr),
+  unless(matchedParamDeclOfAncestorCallExprHasRValueRefType())),
+  this);
 
   // Detect: 's == str.c_str()'  ->  's == str'
   Finder->addMatcher(


Index: clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-cstr.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-cstr.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-cstr.cpp
@@ -205,3 +205,18 @@
 void invalid(const NotAString &s) {
   dummy(s.c_str());
 }
+
+// Test for rvalue std::string.
+void m1(std::string&&) {
+  std::string s;
+
+  m1(s.c_str());
+
+  void (*m1p1)(std::string&&);
+  m1p1 = m1;
+  m1p1(s.c_str());
+
+  using m1tp = void (*)(std::string &&);
+  m1tp m1p2 = m1;
+  m1p2(s.c_str());  
+}
Index: clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
===
--- clang-too

[PATCH] D74033: [clang-tidy] Fix PR#44620 'readability-redundant-string-cstr quick-fix causes invalid code'

[Karasev Nikita via Phabricator via cfe-commits]

f00kat added a comment.

Yes, commit please. Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74033



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

[PATCH] D73090: [clang-tidy] Fix PR#44528 'modernize-use-using and enums'

[Karasev Nikita via Phabricator via cfe-commits]

f00kat updated this revision to Diff 239491.
f00kat added a comment.

Add new line in the end of file checkers/modernize-use-using.cpp


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

https://reviews.llvm.org/D73090

Files:
  clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
  clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
@@ -267,3 +267,7 @@
 // CHECK-MESSAGES: :[[@LINE-2]]:30: warning: use 'using' instead of 'typedef'
 // CHECK-FIXES: using R_t = struct { int a; };
 // CHECK-FIXES-NEXT: using R_p = R_t*;
+
+typedef enum { ea, eb } EnumT;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using EnumT = enum { ea, eb };
Index: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
===
--- clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
+++ clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
@@ -23,7 +23,7 @@
 
   const bool IgnoreMacros;
   SourceLocation LastReplacementEnd;
-  SourceRange LastCxxDeclRange;
+  SourceRange LastTagDeclRange;
   std::string FirstTypedefType;
   std::string FirstTypedefName;
 
Index: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
@@ -25,18 +25,18 @@
 return;
   Finder->addMatcher(typedefDecl(unless(isInstantiated())).bind("typedef"),
  this);
-  // This matcher used to find structs defined in source code within typedefs.
+  // This matcher`s used to find structs/enums defined in source code within 
typedefs.
   // They appear in the AST just *prior* to the typedefs.
-  Finder->addMatcher(cxxRecordDecl(unless(isImplicit())).bind("struct"), this);
+  Finder->addMatcher(cxxRecordDecl(unless(isImplicit())).bind("tagdecl"), 
this);
+  Finder->addMatcher(enumDecl(unless(isImplicit())).bind("tagdecl"), this);
 }
 
 void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
   // Match CXXRecordDecl only to store the range of the last non-implicit full
   // declaration, to later check whether it's within the typdef itself.
-  const auto *MatchedCxxRecordDecl =
-  Result.Nodes.getNodeAs("struct");
-  if (MatchedCxxRecordDecl) {
-LastCxxDeclRange = MatchedCxxRecordDecl->getSourceRange();
+  const auto *MatchedTagDecl = Result.Nodes.getNodeAs("tagdecl");
+  if (MatchedTagDecl) {
+LastTagDeclRange = MatchedTagDecl->getSourceRange();
 return;
   }
 
@@ -96,10 +96,10 @@
   auto Diag = diag(ReplaceRange.getBegin(), UseUsingWarning);
 
   // If typedef contains a full struct/class declaration, extract its full 
text.
-  if (LastCxxDeclRange.isValid() && 
ReplaceRange.fullyContains(LastCxxDeclRange)) {
+  if (LastTagDeclRange.isValid() && 
ReplaceRange.fullyContains(LastTagDeclRange)) {
 bool Invalid;
 Type =
-Lexer::getSourceText(CharSourceRange::getTokenRange(LastCxxDeclRange),
+Lexer::getSourceText(CharSourceRange::getTokenRange(LastTagDeclRange),
  *Result.SourceManager, getLangOpts(), &Invalid);
 if (Invalid)
   return;


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
@@ -267,3 +267,7 @@
 // CHECK-MESSAGES: :[[@LINE-2]]:30: warning: use 'using' instead of 'typedef'
 // CHECK-FIXES: using R_t = struct { int a; };
 // CHECK-FIXES-NEXT: using R_p = R_t*;
+
+typedef enum { ea, eb } EnumT;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using EnumT = enum { ea, eb };
Index: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
===
--- clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
+++ clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
@@ -23,7 +23,7 @@
 
   const bool IgnoreMacros;
   SourceLocation LastReplacementEnd;
-  SourceRange LastCxxDeclRange;
+  SourceRange LastTagDeclRange;
   std::string FirstTypedefType;
   std::string FirstTypedefName;
 
Index: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
@@ -25,18 +25,18 @@
 return;
   Fin

[PATCH] D73464: [clang] Add TagDecl AST matcher

[Karasev Nikita via Phabricator via cfe-commits]

f00kat created this revision.
f00kat added reviewers: aaron.ballman, alexfh, hokein, JonasToth.
f00kat added a project: clang.
Herald added a subscriber: cfe-commits.

In this case https://reviews.llvm.org/D73090#1840501 @aaron.ballman suggest to 
add new AST matcher for tagDecl to avoid using recordDecl and enumDecl together.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73464

Files:
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/ASTMatchersInternal.cpp
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
  clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -184,6 +184,13 @@
   EXPECT_TRUE(notMatches("enum X {};", Matcher));
 }
 
+TEST(TagDecl, MatchesTagDecls) {
+  EXPECT_TRUE(matches("struct X {};", tagDecl(hasName("X";
+  EXPECT_TRUE(matches("class C {};", tagDecl(hasName("C";
+  EXPECT_TRUE(matches("union U {};", tagDecl(hasName("U";
+  EXPECT_TRUE(matches("enum E {};", tagDecl(hasName("E";
+}
+
 TEST(Matcher, UnresolvedLookupExpr) {
   // FIXME: The test is known to be broken on Windows with delayed template
   // parsing.
Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2504,6 +2504,13 @@
   EXPECT_TRUE(notMatches("enum X {};", enumDecl(isScoped(;
 }
 
+TEST(TagDeclKind, MatchesTagDeclKind) {
+  EXPECT_TRUE(matches("struct X {};", tagDecl(isStruct(;
+  EXPECT_TRUE(matches("class C {};", tagDecl(isClass(;
+  EXPECT_TRUE(matches("union U {};", tagDecl(isUnion(;
+  EXPECT_TRUE(matches("enum E {};", tagDecl(isEnum(;
+}
+
 TEST(HasTrailingReturn, MatchesTrailingReturn) {
   EXPECT_TRUE(matches("auto Y() -> int { return 0; }",
   functionDecl(hasTrailingReturn(;
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -208,6 +208,8 @@
   REGISTER_MATCHER(doStmt);
   REGISTER_MATCHER(eachOf);
   REGISTER_MATCHER(elaboratedType);
+  REGISTER_MATCHER(tagDecl);
+  REGISTER_MATCHER(tagType);
   REGISTER_MATCHER(enumConstantDecl);
   REGISTER_MATCHER(enumDecl);
   REGISTER_MATCHER(enumType);
Index: clang/lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -647,6 +647,7 @@
 const internal::VariadicDynCastAllOfMatcher enumDecl;
 const internal::VariadicDynCastAllOfMatcher
 enumConstantDecl;
+const internal::VariadicDynCastAllOfMatcher tagDecl;
 const internal::VariadicDynCastAllOfMatcher cxxMethodDecl;
 const internal::VariadicDynCastAllOfMatcher
 cxxConversionDecl;
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -1194,6 +1194,20 @@
 extern const internal::VariadicDynCastAllOfMatcher
 enumConstantDecl;
 
+/// Matches tag declarations.
+///
+/// Example matches X, Z, U, S, E
+/// \code
+///   class X;
+///   template class Z {};
+///   struct S {};
+///   union U {};
+///   enum E {
+/// A, B, C
+///   };
+/// \endcode
+extern const internal::VariadicDynCastAllOfMatcher tagDecl;
+
 /// Matches method declarations.
 ///
 /// Example matches y
@@ -4845,42 +4859,58 @@
   return InnerMatcher.matches(Node.getType(), Finder, Builder);
 }
 
-/// Matches RecordDecl object that are spelled with "struct."
+/// Matches TagDecl object that are spelled with "struct."
 ///
-/// Example matches S, but not C or U.
+/// Example matches S, but not C, U or E.
 /// \code
 ///   struct S {};
 ///   class C {};
 ///   union U {};
+///   enum E {};
 /// \endcode
-AST_MATCHER(RecordDecl, isStruct) {
+AST_MATCHER(TagDecl, isStruct) {
   return Node.isStruct();
 }
 
-/// Matches RecordDecl object that are spelled with "union."
+/// Matches TagDecl object that are spelled with "union."
 ///
-/// Example matches U, but not C or S.
+/// Example matches U, but not C, S or E.
 /// \code
 ///   struct S {};
 ///   class C {};
 ///   union U {};
+///   enum E {};
 /// \endcode
-AST_MATCHER(RecordDecl, isUnion) {
+AST_MATCHER(TagDecl, isUnion) {
   return Node.isUnion();
 }
 
-/// Matches RecordDecl object that are spelled with "class."
+/// Matches TagDecl object that are spelled with "class."
 ///
-/// Example matches C, but not S or U.
+/// Examp

[PATCH] D73464: [clang] Add TagDecl AST matcher

[Karasev Nikita via Phabricator via cfe-commits]

f00kat updated this revision to Diff 240779.
f00kat added a comment.

1. Fix Registry.cpp
2. Generate AST Matchers doc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73464

Files:
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/ASTMatchersInternal.cpp
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
  clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -184,6 +184,13 @@
   EXPECT_TRUE(notMatches("enum X {};", Matcher));
 }
 
+TEST(TagDecl, MatchesTagDecls) {
+  EXPECT_TRUE(matches("struct X {};", tagDecl(hasName("X";
+  EXPECT_TRUE(matches("class C {};", tagDecl(hasName("C";
+  EXPECT_TRUE(matches("union U {};", tagDecl(hasName("U";
+  EXPECT_TRUE(matches("enum E {};", tagDecl(hasName("E";
+}
+
 TEST(Matcher, UnresolvedLookupExpr) {
   // FIXME: The test is known to be broken on Windows with delayed template
   // parsing.
Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2504,6 +2504,13 @@
   EXPECT_TRUE(notMatches("enum X {};", enumDecl(isScoped(;
 }
 
+TEST(TagDeclKind, MatchesTagDeclKind) {
+  EXPECT_TRUE(matches("struct X {};", tagDecl(isStruct(;
+  EXPECT_TRUE(matches("class C {};", tagDecl(isClass(;
+  EXPECT_TRUE(matches("union U {};", tagDecl(isUnion(;
+  EXPECT_TRUE(matches("enum E {};", tagDecl(isEnum(;
+}
+
 TEST(HasTrailingReturn, MatchesTrailingReturn) {
   EXPECT_TRUE(matches("auto Y() -> int { return 0; }",
   functionDecl(hasTrailingReturn(;
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -492,6 +492,7 @@
   REGISTER_MATCHER(substTemplateTypeParmType);
   REGISTER_MATCHER(switchCase);
   REGISTER_MATCHER(switchStmt);
+  REGISTER_MATCHER(tagDecl);
   REGISTER_MATCHER(tagType);
   REGISTER_MATCHER(templateArgument);
   REGISTER_MATCHER(templateArgumentCountIs);
Index: clang/lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -647,6 +647,7 @@
 const internal::VariadicDynCastAllOfMatcher enumDecl;
 const internal::VariadicDynCastAllOfMatcher
 enumConstantDecl;
+const internal::VariadicDynCastAllOfMatcher tagDecl;
 const internal::VariadicDynCastAllOfMatcher cxxMethodDecl;
 const internal::VariadicDynCastAllOfMatcher
 cxxConversionDecl;
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -1194,6 +1194,20 @@
 extern const internal::VariadicDynCastAllOfMatcher
 enumConstantDecl;
 
+/// Matches tag declarations.
+///
+/// Example matches X, Z, U, S, E
+/// \code
+///   class X;
+///   template class Z {};
+///   struct S {};
+///   union U {};
+///   enum E {
+/// A, B, C
+///   };
+/// \endcode
+extern const internal::VariadicDynCastAllOfMatcher tagDecl;
+
 /// Matches method declarations.
 ///
 /// Example matches y
@@ -4845,42 +4859,58 @@
   return InnerMatcher.matches(Node.getType(), Finder, Builder);
 }
 
-/// Matches RecordDecl object that are spelled with "struct."
+/// Matches TagDecl object that are spelled with "struct."
 ///
-/// Example matches S, but not C or U.
+/// Example matches S, but not C, U or E.
 /// \code
 ///   struct S {};
 ///   class C {};
 ///   union U {};
+///   enum E {};
 /// \endcode
-AST_MATCHER(RecordDecl, isStruct) {
+AST_MATCHER(TagDecl, isStruct) {
   return Node.isStruct();
 }
 
-/// Matches RecordDecl object that are spelled with "union."
+/// Matches TagDecl object that are spelled with "union."
 ///
-/// Example matches U, but not C or S.
+/// Example matches U, but not C, S or E.
 /// \code
 ///   struct S {};
 ///   class C {};
 ///   union U {};
+///   enum E {};
 /// \endcode
-AST_MATCHER(RecordDecl, isUnion) {
+AST_MATCHER(TagDecl, isUnion) {
   return Node.isUnion();
 }
 
-/// Matches RecordDecl object that are spelled with "class."
+/// Matches TagDecl object that are spelled with "class."
 ///
-/// Example matches C, but not S or U.
+/// Example matches C, but not S, U or E.
 /// \code
 ///   struct S {};
 ///   class C {};
 ///   union U {

[PATCH] D73464: [clang] Add TagDecl AST matcher

[Karasev Nikita via Phabricator via cfe-commits]

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

In D73464#1844310 , @aaron.ballman 
wrote:

> LGTM! Do you need someone to commit on your behalf?


Yes, please. I don`t know how :)




Comment at: clang/lib/ASTMatchers/Dynamic/Registry.cpp:212
+  REGISTER_MATCHER(tagDecl);
+  REGISTER_MATCHER(tagType);
   REGISTER_MATCHER(enumConstantDecl);

alexfh wrote:
> There's already `tagType` registration on the line 497. And yes, should the 
> list be sorted, that would be obvious.
Fixed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73464



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

[PATCH] D73464: [clang] Add TagDecl AST matcher

[Karasev Nikita via Phabricator via cfe-commits]

f00kat added a comment.

In D73464#1845511 , @aaron.ballman 
wrote:

> In D73464#1844402 , @f00kat wrote:
>
> > In D73464#1844310 , @aaron.ballman 
> > wrote:
> >
> > > LGTM! Do you need someone to commit on your behalf?
> >
> >
> > Yes, please. I don`t know how :)
>
>
> I'm happy to do so, but I'm not certain what email address you'd like me to 
> use for you (I can't quite suss it out from the phab emails). Can you let me 
> know what address you'd like me to use for the commit attribution?


Yeah, sure. "cc.t...@gmail.com".
I forget to verify email in Phabricator settings.
Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73464



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

[PATCH] D73090: [clang-tidy] Fix PR#44528 'modernize-use-using and enums'

[Karasev Nikita via Phabricator via cfe-commits]

f00kat marked 4 inline comments as done.
f00kat added a comment.

I have changed the code to work with tagDecl. It`s work fine on simple test 
cases such as

  typedef enum { ea, eb } EnumT;

But not work with

  #include 
  typedef enum { ea, eb } EnumT;

It is not related with new tagDecl matcher. I simplify this case:

1. Create file Header.h
2. Header.h

  typedef size_t sometype;

3. main.cpp

  #include "Header.h"
  using EnumT = enum { ea, eb };

It`s happens because in class UseUsingCheck we have variable LastReplacementEnd 
and it may contains SourceLocation for another file.
First time AST matcher trigger on file Header.h and we remember SourceLocation 
for code "typedef size_t sometype". 
Then AST matcher trigger on "using EnumT = enum { ea, eb };" in main.cpp and we 
trying to use the value of LastReplacementEnd storing SourceLocation for 
Header.h.

So this 'if case' does not execute

  if (ReplaceRange.getBegin().isMacroID() ||
 ReplaceRange.getBegin() >= LastReplacementEnd) {

execute this one

  // This is additional TypedefDecl in a comma-separated typedef declaration.
  // Start replacement *after* prior replacement and separate with semicolon.
  ReplaceRange.setBegin(LastReplacementEnd);

Simple fix

  if (ReplaceRange.getBegin().isMacroID() ||
 (Result.SourceManager->getFileID(ReplaceRange.getBegin()) != 
Result.SourceManager->getFileID(LastReplacementEnd)) || \\ new check
 (ReplaceRange.getBegin() >= LastReplacementEnd)) {

But maybe I wrong. I don`t know API well enough.




Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp:28
  this);
-  // This matcher used to find structs defined in source code within typedefs.
+  // This matcher`s used to find structs/enums defined in source code within 
typedefs.
   // They appear in the AST just *prior* to the typedefs.

aaron.ballman wrote:
> It looks like there's a backtick in the comment rather than a quotation mark, 
> that should probably be `matcher's` instead. Also, instead of `struct/enums`, 
> I think it should be `tag declarations` (unions count, for instance).
Ok



Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp:30-31
   // They appear in the AST just *prior* to the typedefs.
-  Finder->addMatcher(cxxRecordDecl(unless(isImplicit())).bind("struct"), this);
+  Finder->addMatcher(cxxRecordDecl(unless(isImplicit())).bind("tagdecl"), 
this);
+  Finder->addMatcher(enumDecl(unless(isImplicit())).bind("tagdecl"), this);
 }

aaron.ballman wrote:
> Rather than matching on these, I think it would make more sense to add a new 
> matcher for `tagDecl()`. It can be local to the check for now, or you can add 
> it to the AST matchers header as a separate patch and then base this patch 
> off that work.
"Add new AST Matcher" - it's the Jedi level :)
Where can I find some manual that describe this? Or maybe some example(git 
commit).

"you can add it to the AST matchers header as a separate patch and then base 
this patch off that work."
So the sequencing will be:
1. I create another new patch with new AST matcher 'tagDecl'
2. Wait until you reviewed it
3. Merge to master
4. Pull latest master
4. Modify code and update this patch
?



Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp:30-31
   // They appear in the AST just *prior* to the typedefs.
-  Finder->addMatcher(cxxRecordDecl(unless(isImplicit())).bind("struct"), this);
+  Finder->addMatcher(cxxRecordDecl(unless(isImplicit())).bind("tagdecl"), 
this);
+  Finder->addMatcher(enumDecl(unless(isImplicit())).bind("tagdecl"), this);
 }

f00kat wrote:
> aaron.ballman wrote:
> > Rather than matching on these, I think it would make more sense to add a 
> > new matcher for `tagDecl()`. It can be local to the check for now, or you 
> > can add it to the AST matchers header as a separate patch and then base 
> > this patch off that work.
> "Add new AST Matcher" - it's the Jedi level :)
> Where can I find some manual that describe this? Or maybe some example(git 
> commit).
> 
> "you can add it to the AST matchers header as a separate patch and then base 
> this patch off that work."
> So the sequencing will be:
> 1. I create another new patch with new AST matcher 'tagDecl'
> 2. Wait until you reviewed it
> 3. Merge to master
> 4. Pull latest master
> 4. Modify code and update this patch
> ?
I add new TagDecl matcher in this patch https://reviews.llvm.org/D73464. Check 
please.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp:274
+// CHECK-FIXES: using EnumT = enum { ea, eb };
\ No newline at end of file


njames93 wrote:
> Can you put a new line here.
Yeah


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

https://reviews.llvm.org/D73090



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
h

[PATCH] D73090: [clang-tidy] Fix PR#44528 'modernize-use-using and enums'

[Karasev Nikita via Phabricator via cfe-commits]

f00kat updated this revision to Diff 241448.
f00kat added a comment.

1. Apply new tagDecl matcher
2. Trying to fix some bug


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73090

Files:
  clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
  clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
@@ -267,3 +267,7 @@
 // CHECK-MESSAGES: :[[@LINE-2]]:30: warning: use 'using' instead of 'typedef'
 // CHECK-FIXES: using R_t = struct { int a; };
 // CHECK-FIXES-NEXT: using R_p = R_t*;
+
+typedef enum { ea, eb } EnumT;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using EnumT = enum { ea, eb };
Index: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
===
--- clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
+++ clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
@@ -23,7 +23,7 @@
 
   const bool IgnoreMacros;
   SourceLocation LastReplacementEnd;
-  SourceRange LastCxxDeclRange;
+  SourceRange LastTagDeclRange;
   std::string FirstTypedefType;
   std::string FirstTypedefName;
 
Index: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
@@ -25,18 +25,17 @@
 return;
   Finder->addMatcher(typedefDecl(unless(isInstantiated())).bind("typedef"),
  this);
-  // This matcher used to find structs defined in source code within typedefs.
+  // This matcher used to find tag declarations in source code within typedefs.
   // They appear in the AST just *prior* to the typedefs.
-  Finder->addMatcher(cxxRecordDecl(unless(isImplicit())).bind("struct"), this);
+  Finder->addMatcher(tagDecl(unless(isImplicit())).bind("tagdecl"), this);
 }
 
 void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
   // Match CXXRecordDecl only to store the range of the last non-implicit full
   // declaration, to later check whether it's within the typdef itself.
-  const auto *MatchedCxxRecordDecl =
-  Result.Nodes.getNodeAs("struct");
-  if (MatchedCxxRecordDecl) {
-LastCxxDeclRange = MatchedCxxRecordDecl->getSourceRange();
+  const auto *MatchedTagDecl = Result.Nodes.getNodeAs("tagdecl");
+  if (MatchedTagDecl) {
+LastTagDeclRange = MatchedTagDecl->getSourceRange();
 return;
   }
 
@@ -72,7 +71,8 @@
   // the end of the current TypedefDecl definition.
   std::string Using = "using ";
   if (ReplaceRange.getBegin().isMacroID() ||
-  ReplaceRange.getBegin() >= LastReplacementEnd) {
+  (Result.SourceManager->getFileID(ReplaceRange.getBegin()) != 
Result.SourceManager->getFileID(LastReplacementEnd)) ||
+  (ReplaceRange.getBegin() >= LastReplacementEnd)) {
 // This is the first (and possibly the only) TypedefDecl in a typedef. Save
 // Type and Name in case we find subsequent TypedefDecl's in this typedef.
 FirstTypedefType = Type;
@@ -95,11 +95,11 @@
 
   auto Diag = diag(ReplaceRange.getBegin(), UseUsingWarning);
 
-  // If typedef contains a full struct/class declaration, extract its full 
text.
-  if (LastCxxDeclRange.isValid() && 
ReplaceRange.fullyContains(LastCxxDeclRange)) {
+  // If typedef contains a full tag declaration, extract its full text.
+  if (LastTagDeclRange.isValid() && 
ReplaceRange.fullyContains(LastTagDeclRange)) {
 bool Invalid;
 Type = std::string(
-Lexer::getSourceText(CharSourceRange::getTokenRange(LastCxxDeclRange),
+Lexer::getSourceText(CharSourceRange::getTokenRange(LastTagDeclRange),
  *Result.SourceManager, getLangOpts(), &Invalid));
 if (Invalid)
   return;


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
@@ -267,3 +267,7 @@
 // CHECK-MESSAGES: :[[@LINE-2]]:30: warning: use 'using' instead of 'typedef'
 // CHECK-FIXES: using R_t = struct { int a; };
 // CHECK-FIXES-NEXT: using R_p = R_t*;
+
+typedef enum { ea, eb } EnumT;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using EnumT = enum { ea, eb };
Index: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
===
--- clang-tools-extra/cla