[PATCH] D71174: [clang-tidy] new check: bugprone-signed-char-misuse

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added inline comments.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:66
bugprone-posix-return
+   bugprone-signed-char-misuse
bugprone-sizeof-container

list.rst has changed, you should try to rebase your patch!



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71174



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

[PATCH] D71939: clang-format: fix conflict between FormatStyle::BWACS_MultiLine and BeforeCatch/BeforeElse

[Pavlo Shkrabliuk via Phabricator via cfe-commits]

pastey updated this revision to Diff 235466.
pastey added a comment.

Bouska, thank you! Don't know how I missed that failing test. Never commit on 
friday :)

However, seeing this test result in a browser window gave me that "a-ha". I 
understood that I had wrong perception of what MultiLine feature is all about. 
As I wrote before, I would expect it to format code like this:

  if (foo ||
  bar)
  {
doSomething();
  }
  else {
doSomethingElse();
  }

In my head it didn't have anything to do with ColumnLimit – it was all about 
more than one line in 'if' condition expression.

After I understood that MultiLine is bound to ColumnLimit, I could understand 
what was going on @ line 308 on UnwrappedLineFormatter.cpp.

Also found that UnwrappedLine actually wraps every line, which is ... confusing.

Uploaded the new diff. Removed my previous "fix" from UnwrappedLineParser.cpp. 
Added special case for else/catch in UnwrappedLineFormatter.cpp. Tests pass.


Repository:
  rC Clang

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

https://reviews.llvm.org/D71939

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


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -1565,6 +1565,41 @@
 "  baz();\n"
 "}",
 format("try{foo();}catch(Exception&bar){baz();}", Style));
+  Style.ColumnLimit =
+  40; // to concentrate at brace wrapping, not line wrap due to column 
limit
+  EXPECT_EQ("try {\n"
+"  foo();\n"
+"} catch (Exception &bar) {\n"
+"  baz();\n"
+"}",
+format("try{foo();}catch(Exception&bar){baz();}", Style));
+  Style.ColumnLimit =
+  20; // to concentrate at brace wrapping, not line wrap due to column 
limit
+
+  Style.BraceWrapping.BeforeElse = true;
+  EXPECT_EQ(
+  "if (foo) {\n"
+  "  bar();\n"
+  "}\n"
+  "else if (baz ||\n"
+  " quux)\n"
+  "{\n"
+  "  foobar();\n"
+  "}\n"
+  "else {\n"
+  "  barbaz();\n"
+  "}",
+  format("if(foo){bar();}else if(baz||quux){foobar();}else{barbaz();}",
+ Style));
+
+  Style.BraceWrapping.BeforeCatch = true;
+  EXPECT_EQ("try {\n"
+"  foo();\n"
+"}\n"
+"catch (...) {\n"
+"  baz();\n"
+"}",
+format("try{foo();}catch(...){baz();}", Style));
 }
 
 
//===--===//
@@ -14881,7 +14916,7 @@
   verifyFormat("auto lambda = [&a = a]() { a = 2; };", AlignStyle);
 }
 
- TEST_F(FormatTest, SpacesInConditionalStatement) {
+TEST_F(FormatTest, SpacesInConditionalStatement) {
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpacesInConditionalStatement = true;
   verifyFormat("for ( int i = 0; i; i++ )\n  continue;", Spaces);
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -326,6 +326,14 @@
   FormatStyle::BWACS_Always)
  ? tryMergeSimpleBlock(I, E, Limit)
  : 0;
+} else if (I[1]->First->is(tok::l_brace) &&
+   TheLine->First->isOneOf(tok::kw_else, tok::kw_catch) &&
+   Style.BraceWrapping.AfterControlStatement ==
+   FormatStyle::BWACS_MultiLine) {
+  return (Style.ColumnLimit == 0 ||
+  TheLine->Last->TotalLength <= Style.ColumnLimit)
+ ? 1
+ : 0;
 }
 // Try to merge either empty or one-line block if is precedeed by control
 // statement token


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -1565,6 +1565,41 @@
 "  baz();\n"
 "}",
 format("try{foo();}catch(Exception&bar){baz();}", Style));
+  Style.ColumnLimit =
+  40; // to concentrate at brace wrapping, not line wrap due to column limit
+  EXPECT_EQ("try {\n"
+"  foo();\n"
+"} catch (Exception &bar) {\n"
+"  baz();\n"
+"}",
+format("try{foo();}catch(Exception&bar){baz();}", Style));
+  Style.ColumnLimit =
+  20; // to concentrate at brace wrapping, not line wrap due to column limit
+
+  Style.BraceWrapping.BeforeElse = true;
+  EXPECT_EQ(
+  "if (foo) {\n"
+  "  bar();\n"
+  "}\n"
+  "else if (baz ||\n"
+  " quux)\n"
+  "{\n"
+  "  foobar();\n"
+  "}\n"
+  "else {\n"
+  "  barbaz();\n"
+  "}",
+  format("if(foo){bar();}else if(baz||quux){foobar();}else{barbaz();}",
+ Style));
+
+  Style

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Kocsis Ábel via Phabricator via cfe-commits]

abelkocsis updated this revision to Diff 235469.
abelkocsis marked an inline comment as done.
abelkocsis added a comment.

Checker fixes, updates test file adding test cases for `doWhile` and `for` 
statements, test headers deleted.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-spuriously-wake-up-functions.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-con36-c.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-con54-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp
@@ -0,0 +1,270 @@
+// RUN: %check_clang_tidy %s bugprone-spuriously-wake-up-functions %t -- --
+#define NULL 0
+
+namespace std {
+using intmax_t = int;
+
+template 
+class ratio {
+public:
+  static constexpr intmax_t num = 0;
+  static constexpr intmax_t den = 0;
+  typedef ratio type;
+};
+typedef ratio<1, 1000> milli;
+namespace chrono {
+
+template >
+class duration {
+public:
+  using rep = Rep;
+  using period = Period;
+
+public:
+  constexpr duration() = default;
+  template 
+  constexpr explicit duration(const Rep2 &r);
+  template 
+  constexpr duration(const duration &d);
+  ~duration() = default;
+  duration(const duration &) = default;
+};
+
+template 
+class time_point {
+public:
+  using clock = Clock;
+  using duration = Duration;
+
+public:
+  constexpr time_point();
+  constexpr explicit time_point(const duration &d);
+  template 
+  constexpr time_point(const time_point &t);
+};
+
+using milliseconds = duration;
+
+class system_clock {
+public:
+  typedef milliseconds duration;
+  typedef duration::rep rep;
+  typedef duration::period period;
+  typedef chrono::time_point time_point;
+
+  static time_point now() noexcept;
+};
+} // namespace chrono
+
+class mutex;
+template 
+class unique_lock {
+public:
+  typedef Mutex mutex_type;
+
+  unique_lock() noexcept;
+  explicit unique_lock(mutex_type &m);
+};
+
+class mutex {
+public:
+  constexpr mutex() noexcept;
+  ~mutex();
+  mutex(const mutex &) = delete;
+  mutex &operator=(const mutex &) = delete;
+};
+
+enum class cv_status {
+  no_timeout,
+  timeout
+};
+
+class condition_variable {
+public:
+  condition_variable();
+  ~condition_variable();
+  condition_variable(const condition_variable &) = delete;
+
+  void wait(unique_lock &lock);
+  template 
+  void wait(unique_lock &lock, Predicate pred);
+  template 
+  cv_status wait_until(unique_lock &lock,
+   const chrono::time_point &abs_time){};
+  template 
+  bool wait_until(unique_lock &lock,
+  const chrono::time_point &abs_time,
+  Predicate pred){};
+  template 
+  cv_status wait_for(unique_lock &lock,
+ const chrono::duration &rel_time){};
+  template 
+  bool wait_for(unique_lock &lock,
+const chrono::duration &rel_time,
+Predicate pred){};
+};
+
+} // namespace std
+
+struct Node1 {
+  void *Node1;
+  struct Node1 *next;
+};
+
+static Node1 list;
+static std::mutex m;
+static std::condition_variable condition;
+
+void consume_list_element(std::condition_variable &condition) {
+  std::unique_lock lk(m);
+
+  if (list.next == nullptr) {
+condition.wait(lk);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait' should be placed inside a while statement or used with a conditional parameter [bugprone-spuriously-wake-up-functions]
+  }
+
+  while (list.next == nullptr) {
+condition.wait(lk);
+  }
+
+  do {
+condition.wait(lk);
+  } while (list.next == nullptr);
+
+  for (;; list.next == nullptr) {
+condition.wait(lk);
+  }
+
+  if (list.next == nullptr) {
+while (list.next == nullptr) {
+  condition.wait(lk);
+}
+  }
+
+  if (list.next == nullptr) {
+do {
+  condition.wait(lk);
+} while (list.next == nullptr);
+  }
+
+  if (list.next == nullptr) {
+for (;; list.next == nullptr) {
+  condition.wait(lk);
+}
+  }
+  using durtype = std::chrono::duration;
+  durtype dur = std::chrono::duration();
+  if (list.next == nullptr) {
+condition.wait_for(lk, dur);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait_for' should be placed inside a while statement or used with a conditional parameter [bugprone-spuriously-wake-up-f

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Kocsis Ábel via Phabricator via cfe-commits]

abelkocsis marked 11 inline comments as done.
abelkocsis added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp:2
+// RUN: %check_clang_tidy %s bugprone-spuriously-wake-up-functions %t -- -- -I 
%S/Inputs/bugprone-spuriously-wake-up-functions/
+#include "condition_variable.h"
+#define NULL 0

aaron.ballman wrote:
> Are you planning to use these declarations in multiple test files? If not, 
> then we typically put all of the header code into the test file rather than 
> added as explicit files.
I'm not sure yet, so I move it into the test file.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876



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

[PATCH] D71174: [clang-tidy] new check: bugprone-signed-char-misuse

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas marked 2 inline comments as done.
ztamas added inline comments.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:66
bugprone-posix-return
+   bugprone-signed-char-misuse
bugprone-sizeof-container

sylvestre.ledru wrote:
> list.rst has changed, you should try to rebase your patch!
> 
I rebased the patch. I added medium severity for this check because the CERT 
rule has the same severity.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71174



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

[PATCH] D71174: [clang-tidy] new check: bugprone-signed-char-misuse

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas updated this revision to Diff 235470.
ztamas added a comment.

Rebase patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71174

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse-fsigned-char.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse-funsigned-char.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse-with-option.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp
@@ -0,0 +1,123 @@
+// RUN: %check_clang_tidy %s bugprone-signed-char-misuse %t
+
+///
+/// Test cases correctly caught by the check.
+
+int SimpleVarDeclaration() {
+  signed char CCharacter = -5;
+  int NCharacter = CCharacter;
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: singed char -> integer ('int') conversion; consider to cast to unsigned char first. [bugprone-signed-char-misuse]
+
+  return NCharacter;
+}
+
+int SimpleAssignment() {
+  signed char CCharacter = -5;
+  int NCharacter;
+  NCharacter = CCharacter;
+  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: singed char -> integer ('int') conversion; consider to cast to unsigned char first. [bugprone-signed-char-misuse]
+
+  return NCharacter;
+}
+
+int CStyleCast() {
+  signed char CCharacter = -5;
+  int NCharacter;
+  NCharacter = (int)CCharacter;
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: singed char -> integer ('int') conversion; consider to cast to unsigned char first. [bugprone-signed-char-misuse]
+
+  return NCharacter;
+}
+
+int StaticCast() {
+  signed char CCharacter = -5;
+  int NCharacter;
+  NCharacter = static_cast(CCharacter);
+  // CHECK-MESSAGES: [[@LINE-1]]:33: warning: singed char -> integer ('int') conversion; consider to cast to unsigned char first. [bugprone-signed-char-misuse]
+
+  return NCharacter;
+}
+
+int FunctionalCast() {
+  signed char CCharacter = -5;
+  int NCharacter;
+  NCharacter = int(CCharacter);
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: singed char -> integer ('int') conversion; consider to cast to unsigned char first. [bugprone-signed-char-misuse]
+
+  return NCharacter;
+}
+
+int NegativeConstValue() {
+  const signed char CCharacter = -5;
+  int NCharacter = CCharacter;
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: singed char -> integer ('int') conversion; consider to cast to unsigned char first. [bugprone-signed-char-misuse]
+
+  return NCharacter;
+}
+
+int CharPointer(signed char *CCharacter) {
+  int NCharacter = *CCharacter;
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: singed char -> integer ('int') conversion; consider to cast to unsigned char first. [bugprone-signed-char-misuse]
+
+  return NCharacter;
+}
+
+///
+/// Test cases correctly ignored by the check.
+
+int UnsignedCharCast() {
+  unsigned char CCharacter = 'a';
+  int NCharacter = CCharacter;
+
+  return NCharacter;
+}
+
+int PositiveConstValue() {
+  const signed char CCharacter = 5;
+  int NCharacter = CCharacter;
+
+  return NCharacter;
+}
+
+// singed char -> integer cast is not the direct child of declaration expression.
+int DescendantCast() {
+  signed char CCharacter = 'a';
+  int NCharacter = 10 + CCharacter;
+
+  return NCharacter;
+}
+
+// singed char -> integer cast is not the direct child of assignment expression.
+int DescendantCastAssignment() {
+  signed char CCharacter = 'a';
+  int NCharacter;
+  NCharacter = 10 + CCharacter;
+
+  return NCharacter;
+}
+
+// bool is an integer type in clang; make sure to ignore it.
+bool BoolVarDeclaration() {
+  signed char CCharacter = 'a';
+  bool BCharacter = CCharacter == 'b';
+
+  return BCharacter;
+}
+
+// bool is an integer type in clang; make sure to ignore it.
+bool BoolAssignment() {
+  signed char CCharacter = 'a';
+  bool BCharacter;
+  BCharacter = CCharacter == 'b';
+
+  return BCharacter;
+}
+
+// char is an integer type in clang; make sure to ignore it.
+unsigned char CharToCharCast() {
+  signed char SCCharacter = 'a';
+  unsigned char USCharacter;
+  USCharacter = SCCharacter;
+
+  return USCharacter;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse-with-option.cpp
=

[PATCH] D71939: clang-format: fix conflict between FormatStyle::BWACS_MultiLine and BeforeCatch/BeforeElse

[Pablo Martin-Gomez via Phabricator via cfe-commits]

Bouska added a comment.

I'm not sure creating a special case for else/catch is the best idea. I would 
seem better to change the control statement @ line 309 to add it the case where 
we break before else or catch (add a new || test in the middle condition). I'm 
really new to this code too, so I don't know if it's better to check that 
previous line as a right curly brace and that BeforeElse/Catch is true and that 
first token of the line is else or catch or if just checking that the first 
token of the line is else or catch (or a mix in between). I'll let a more 
senior reviewer advise on this.


Repository:
  rC Clang

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

https://reviews.llvm.org/D71939



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

Re: [clang] d801823 - Revert "CWG2352: Allow qualification conversions during reference binding."

[Richard Smith via cfe-commits]

On Fri, 27 Dec 2019 at 15:59, David Blaikie via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> On Fri, Dec 27, 2019 at 3:48 PM Richard Smith 
> wrote:
>
>> On Fri, 27 Dec 2019 at 12:27, David Blaikie via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>>
>>> Author: David Blaikie
>>> Date: 2019-12-27T12:27:20-08:00
>>> New Revision: d8018233d1ea4234de68d5b4593abd773db79484
>>>
>>> URL:
>>> https://github.com/llvm/llvm-project/commit/d8018233d1ea4234de68d5b4593abd773db79484
>>> DIFF:
>>> https://github.com/llvm/llvm-project/commit/d8018233d1ea4234de68d5b4593abd773db79484.diff
>>>
>>> LOG: Revert "CWG2352: Allow qualification conversions during reference
>>> binding."
>>>
>>> This reverts commit de21704ba96fa80d3e9402f12c6505917a3885f4.
>>>
>>> Regressed/causes this to error due to ambiguity:
>>>
>>>   void f(const int * const &);
>>>   void f(int *);
>>>   int main() {
>>> int * x;
>>> f(x);
>>>   }
>>>
>>> (in case it's important - the original case where this turned up was a
>>> member function overload in a class template with, essentially:
>>>
>>>   f(const T1&)
>>>   f(T2*)
>>>
>>> (where T1 == X const *, T2 == X))
>>>
>>> It's not super clear to me if this ^ is expected behavior, in which case
>>> I'm sorry about the revert & happy to look into ways to fix the original
>>> code.
>>>
>>
>> I believe the new Clang behavior here is correct according to the
>> standard wording.
>>
>
> Ah, OK. Thanks for looking/checking!
>
>
>> However, GCC trunk also implements CWG2352 and accepts this,
>>
>
> Right, I should've mentioned I checked against GCC trunk. The other case
> that showed up with this change:
>
> template
> void f(T&&);
> void f() {
>   int *i;
>   f(i);
> }
>
> that one failed with both GCC and Clang (& I've written up a fix for it
> internally) & /seems/ roughly like what I believe is to be correct behavior
> form the compiler/new patch here (& consistent with GCC, giving it a bit
> more a vote for "this seems like good behavior")
>

Yes, I think this one is correct behavior.


> and there's a natural-seeming way to get that result, so I'm going to take
>> this back to the core reflector and see if people agree that this ought to
>> remain valid.
>>
>
> OK - happy to help with internal fixes if/when that's needed (I think it
> might just be the one, though - so maybe that goes to show this isn't the
> worst behavior)
>
>
>>
>>
>>> Added:
>>>
>>>
>>> Modified:
>>> clang/include/clang/Basic/DiagnosticSemaKinds.td
>>> clang/lib/Sema/SemaExprCXX.cpp
>>> clang/lib/Sema/SemaInit.cpp
>>> clang/lib/Sema/SemaOverload.cpp
>>> clang/test/CXX/drs/dr23xx.cpp
>>> clang/test/CXX/drs/dr4xx.cpp
>>> clang/test/SemaObjCXX/arc-overloading.mm
>>> clang/test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
>>> clang/www/cxx_dr_status.html
>>> clang/www/make_cxx_dr_status
>>>
>>> Removed:
>>>
>>>
>>>
>>>
>>> 
>>> diff  --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
>>> b/clang/include/clang/Basic/DiagnosticSemaKinds.td
>>> index b86abd0db73e..54299a0409fd 100644
>>> --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
>>> +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
>>> @@ -1933,8 +1933,7 @@ def err_lvalue_reference_bind_to_unrelated : Error<
>>>"cannot bind to a value of unrelated type}1,2">;
>>>  def err_reference_bind_drops_quals : Error<
>>>"binding reference %
>>> diff {of type $ to value of type $|to value}0,1 "
>>> -  "%select{drops %3 qualifier%plural{1:|2:|4:|:s}4|changes address
>>> space|"
>>> -  "not permitted due to incompatible qualifiers}2">;
>>> +  "%select{drops %3 qualifier%plural{1:|2:|4:|:s}4|changes address
>>> space}2">;
>>>  def err_reference_bind_failed : Error<
>>>"reference %
>>> diff {to %select{type|incomplete type}1 $ could not bind to an "
>>>"%select{rvalue|lvalue}2 of type $|could not bind to
>>> %select{rvalue|lvalue}2 of "
>>>
>>> diff  --git a/clang/lib/Sema/SemaExprCXX.cpp
>>> b/clang/lib/Sema/SemaExprCXX.cpp
>>> index cd78f096bb22..cfb3a05e9c14 100644
>>> --- a/clang/lib/Sema/SemaExprCXX.cpp
>>> +++ b/clang/lib/Sema/SemaExprCXX.cpp
>>> @@ -5864,8 +5864,6 @@ QualType
>>> Sema::CXXCheckConditionalOperands(ExprResult &Cond, ExprResult &LHS,
>>>//   one of the operands is reference-compatible with the other, in
>>> order
>>>//   to support conditionals between functions
>>> diff ering in noexcept. This
>>>//   will similarly cover
>>> diff erence in array bounds after P0388R4.
>>> -  // FIXME: If LTy and RTy have a composite pointer type, should we
>>> convert to
>>> -  //   that instead?
>>>ExprValueKind LVK = LHS.get()->getValueKind();
>>>ExprValueKind RVK = RHS.get()->getValueKind();
>>>if (!Context.hasSameType(LTy, RTy) &&
>>>
>>> diff  --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
>>> index ef4fa827064e..94d524a63f5a 100644
>>> --- a/clang/lib/Sema/SemaInit.cpp
>>

[PATCH] D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal updated this revision to Diff 235474.
vingeldal marked 12 inline comments as done.
vingeldal added a comment.

Updating D70265 : [clang-tidy] Add clang tidy 
check I.2 to cppcoreguidelines

- Updated list.rst
- changed: "pointed to" -> "pointed-to"
- CHanged option name: NoMembers -> IgnoreDataMembers
- Moved matcher declaration into if-statement
- Fixed some formatting issues
- Removed forgotten FIXME-comments that were no longer relevant


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70265

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables-IgnoreDataMembers.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
@@ -0,0 +1,268 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-non-const-global-variables %t
+
+int nonConstInt = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: variable 'nonConstInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+
+int &nonConstIntReference = nonConstInt;
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: variable 'nonConstIntReference' provides global access to non-const type, consider making the referenced data const [cppcoreguidelines-avoid-non-const-global-variables]
+
+int *pointerToNonConstInt = &nonConstInt;
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: variable 'pointerToNonConstInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES: :[[@LINE-2]]:6: warning: variable 'pointerToNonConstInt' provides global access to non-const type, consider making the pointed-to data const [cppcoreguidelines-avoid-non-const-global-variables]
+
+int *const constPointerToNonConstInt = &nonConstInt;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'constPointerToNonConstInt' provides global access to non-const type, consider making the pointed-to data const [cppcoreguidelines-avoid-non-const-global-variables]
+
+namespace namespace_name {
+int nonConstNamespaceInt = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: variable 'nonConstNamespaceInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+
+const int constNamespaceInt = 0;
+} // namespace namespace_name
+
+const int constInt = 0;
+
+const int *pointerToConstInt = &constInt;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'pointerToConstInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+
+const int *const constPointerToConstInt = &constInt;
+
+const int &constReferenceToConstInt = constInt;
+
+constexpr int constexprInt = 0;
+
+int function() {
+  int nonConstReturnValue = 0;
+  return nonConstReturnValue;
+}
+
+namespace {
+int nonConstAnonymousNamespaceInt = 0;
+}
+
+class DummyClass {
+public:
+  int nonConstMemberVariable = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: member variable 'nonConstMemberVariable' is globally accessible and non-const, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+  const int constMemberVariable = 0;
+  int *const constMemberPointerToNonConst = &nonConstInt;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member variable 'constMemberPointerToNonConst' provides global access to non-const type, consider making the pointed-to data const [cppcoreguidelines-avoid-non-const-global-variables]
+  int *memberPointerToNonConst = &nonConstInt;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member variable 'memberPointerToNonConst' is globally accessible and non-const, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+  // CHECK-MESSAGES: :[[@LINE-2]]:8: warning: member variable 'memberPointerToNonConst' provides global access to non-const type, consider making the pointed-to data const [cppcoreguidelines-avoid-non-const-global-variables]
+  const int *const constMemberPointerToConst = &constInt;
+  int &nonConstMemberReferenceToNonCons

[PATCH] D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:64
+
+  if (Variable) {
+diag(Variable->getLocation(), "variable %0 is non-const and globally "

aaron.ballman wrote:
> vingeldal wrote:
> > JonasToth wrote:
> > > each of those `if`s should `return` early, or could multiple match?
> > > If only one can match the structure could be changed a bit to
> > > ```
> > > if (const auto* Variable = 
> > > Result.Nodes.getNodeAs("non-const_variable")) {
> > > diag(...);
> > > return;
> > > }
> > > ```
> > > 
> > > If you change the order of the `if`s in the order of matches you expect 
> > > to happen most, this should be a bit faster as well, as retrieving the 
> > > matches introduces some overhead that is not relevant in the current 
> > > situation.
> > > 
> > > If you keep only one statement within the `if` you should ellide the 
> > > braces, per coding convention.
> > There could be multiple matches but there could still be some early returns.
> > An example of a multi match would be:
> > 
> > namespace {
> > int i = 0;
> > }
> > int * i_ptr = &i;
> > 
> > There would be two warnings for i_ptr, since it is:
> >  1. A global non-const variable it self and...
> >  2. because it globally exposes the non-const variable i.
> > 
> > I'll add early returns where possible.
> > 
> > ...Now that I think about it I realize I'v missed checking for member 
> > variables referencing or pointing to non-const data,
> > I'll add that tigether with some more testing.
> Based on my reading of the C++ core guideline, I think there should be a 
> different two diagnostics. One for the declaration of `i` and one for the 
> declaration of `i_ptr`, because of this from the rule:
> 
> > The rule against global variables applies to namespace scope variables as 
> > well.
If the variable i was in a **named** namespace it would be matched by a 
diagnostic, i_ptr is matched by another diagnostic for pointer providing global 
access to non-const data (as well as the matcher for global non-const variables)



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:29
+  unless(anyOf(
+  isConstexpr(), hasDeclContext(namespaceDecl(isAnonymous())),
+  hasType(isConstQualified()),

aaron.ballman wrote:
> Why are you filtering out anonymous namespaces?
If it's in an anonymous namespace it's no longer globally accessible, it's only 
accessible within the file it is declared.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:123-124
+   "member variable %0 provides global access to non-const type, "
+   "consider "
+   "making the referenced data const")
+  << MemberReference; // FIXME: Add fix-it hint to MemberReference

aaron.ballman wrote:
> Can re-flow this string literal.
Sorry, I don't understand what you mean by re-flow.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst:52
+
+.. option:: NoMembers (default = 0)
+

aaron.ballman wrote:
> Rather than `NoMembers`, how about going with `IgnoreDataMembers`?
Yes, that's much better, thanks.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:198
cppcoreguidelines-avoid-magic-numbers (redirects to 
readability-magic-numbers) 
+   cppcoreguidelines-avoid-non-const-global-variables
cppcoreguidelines-c-copy-assignment-signature (redirects to 
misc-unconventional-assign-operator) 


sylvestre.ledru wrote:
> list.rst changed, you should update this!
> Thanks
> 
I'm a bit concerned with this previous change of of list.rst.

Now that I need to add a check to this file, I don't know what severity to give 
it. I can't find any definition of severity levels and I really want to avoid 
getting into a long discussion, or having different reviewers not agreeing on 
my patch, concerning what severity we should give this check.
Is there any way I can find some kind of guideline on how to set the severity 
to avoid an opinionated discussion about severity level?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70265



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

[PATCH] D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:29
+  unless(anyOf(
+  isConstexpr(), hasDeclContext(namespaceDecl(isAnonymous())),
+  hasType(isConstQualified()),

vingeldal wrote:
> aaron.ballman wrote:
> > Why are you filtering out anonymous namespaces?
> If it's in an anonymous namespace it's no longer globally accessible, it's 
> only accessible within the file it is declared.
It is, however, at namespace scope which is what the C++ Core Guideline 
suggests to diagnose. Do you have evidence that this is the interpretation the 
guideline authors were looking for (perhaps they would like to update their 
suggested guidance for diagnosing)?

There are two dependency issues to global variables -- one is surprising 
linkage interactions (where the extern nature of the symbol is an issue across 
module boundaries) and the other is surprising name lookup behavior within the 
TU (where the global nature of the symbol is an issue). e.g.,
```
namespace {
int ik;
}

void f() {
  for (int ij = 0; ik < 10; ij++) { // Oops, typo, but still compiles
  }
}
```
That's why I think the guideline purposefully does not exclude things like 
anonymous namespaces.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:123-124
+   "member variable %0 provides global access to non-const type, "
+   "consider "
+   "making the referenced data const")
+  << MemberReference; // FIXME: Add fix-it hint to MemberReference

vingeldal wrote:
> aaron.ballman wrote:
> > Can re-flow this string literal.
> Sorry, I don't understand what you mean by re-flow.
Sorry for not being clear -- the string literal spans three lines when it only 
needs to span two by re-writing the literal. e.g.,
```
"member variable %0 provides global access to non-const type, "
"consider making the pointed-to data const"
```



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:198
cppcoreguidelines-avoid-magic-numbers (redirects to 
readability-magic-numbers) 
+   cppcoreguidelines-avoid-non-const-global-variables
cppcoreguidelines-c-copy-assignment-signature (redirects to 
misc-unconventional-assign-operator) 


vingeldal wrote:
> sylvestre.ledru wrote:
> > list.rst changed, you should update this!
> > Thanks
> > 
> I'm a bit concerned with this previous change of of list.rst.
> 
> Now that I need to add a check to this file, I don't know what severity to 
> give it. I can't find any definition of severity levels and I really want to 
> avoid getting into a long discussion, or having different reviewers not 
> agreeing on my patch, concerning what severity we should give this check.
> Is there any way I can find some kind of guideline on how to set the severity 
> to avoid an opinionated discussion about severity level?
I'd like to follow that discussion so that I can be consistent with my review 
advice, too. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70265



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

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.cpp:60
+  .bind("wait"));
+  Finder->addMatcher(
+  ifStmt(anyOf(

Sorry for not noticing this earlier, but I think you can make two calls to 
`addMatcher()`, one for the C check and one for the C++ check. Then you can 
register only the matcher needed for the language mode you are currently in. 
e.g.,
```
if (getLangOpts().CPlusPlus)
  // Check for CON54-CPP
else
  // Check for CON36-C
```
This should make the check slightly more efficient because of the mutually 
exclusive nature of these APIs without changing the behavior.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:75
`bugprone-sizeof-expression `_, , "high"
+   `bugprone-spuriously-wake-up-functions 
`_, , ""
`bugprone-string-constructor `_, "Yes", 
"high"

sylvestre.ledru wrote:
> Could you try to evaluate the severity? :)
> thanks
> 
Given that these are essentially CERT rules, I'd go with the CERT severity for 
them -- however, I say that only because I'm not certain what approach is 
supposed to be taken for these severity markings.

CERT marks these two rules as having a low severity, FWIW.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876



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

[PATCH] D71174: [clang-tidy] new check: bugprone-signed-char-misuse

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp:97-98
+  diag(CastExpression->getBeginLoc(),
+   "singed char -> integer (%0) conversion; "
+   "consider to cast to unsigned char first.")
+  << *IntegerType;

How about: `'signed char' to %0 conversion; consider casting to 'unsigned char' 
first`?

Also, should we have a fix-it to automatically insert the cast to `unsigned 
char` in the proper location for the user?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst:25
+See also:
+`STR34-C. Cast characters to unsigned char before converting to larger integer 
sizes
+`_

ztamas wrote:
> aaron.ballman wrote:
> > Should this check also be registered in the CERT module?
> This check does not cover the whole CERT description. I guess a cert check 
> should catch all bugous code related to the CERT issue, right?
So long as this covers a decent amount of the CERT rule, I think it is fine to 
document the areas where we don't quite match the CERT rule.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst:21
+by default and so it is caught by this check or not. To change the default 
behavior
+you can use -funsigned-char and -fsigned-char compilation options.
+

Backticks around the command line options.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst:23
+
+By now, this check is limited to assignments and variable declarations,
+where a ``signed char`` is assigned to an integer variable. There are other

By now -> Currently



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst:25
+where a ``signed char`` is assigned to an integer variable. There are other
+use cases where the same misinterpretation might lead to similar bugous
+behavior.

bugous -> bogus


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71174



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D71846#1796883 , @njames93 wrote:

> So I wasn't happy with the vagueness of the else after return check 
> implementation. Almost finished rewriting the check to properly handle the if 
> statements with condition variables based on scope restrictions and where 
> decls are used etc.
>
>   int *varInitAndCondition() {
> if (int *X = g(); X != nullptr) {
>   return X;
> } else {
>   h(&X);
>   return X;
> }
>   }
>   int *varInitAndConditionUnusedInElseWithDecl() {
> int *Y = g();
> if (int *X = g(); X != nullptr) {
>   return X;
> } else {
>   int *Y = g();
>   h(&Y);
> }
> return Y;
>   }
>
>
> transforms to
>
>   int *varInitAndCondition() {
> int *X = g();
> if (X != nullptr) {
>   return X;
> }
> h(&X);
> return X;
>   }
>   int *varInitAndConditionUnusedInElseWithDecl() {
> int *Y = g();
> if (int *X = g(); X != nullptr) {
>   return X;
> } else {
>   int *Y = g();
>   h(&Y);
> }
> return Y;
>   }
>
>
> There's a few more test cases but that's the general idea. I'm having a 
> little trouble writing the test cases as I can't run them on my windows 
> machine to verify they report correctly


Hmm, I'm not convinced the new behavior is better in all cases, and I suspect 
this may boil down to user preference (suggesting we may want an option to 
control the behavior). There are competing stylistic decisions to be made for 
`varInitAndCondition()` -- one is the `else` after a `return`, but the other is 
the scope of the variable `X`. The behavior now forces the user to prefer 
removing the `else` after the `return` over hiding the variable `X` in a 
logical scope. For the contrived example provided, this isn't a huge deal 
because the function is small. However, in real-world code with larger function 
bodies, I can see wanting to prefer the name hiding. WDYT?


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

https://reviews.llvm.org/D71846



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

I like the goal of this patch and the simplifications it does. I don't feel 
qualified to do a full review.




Comment at: clang/include/clang/AST/Expr.h:126
+if (TD)
+  D = D | DependencyFlags::Type;
+if (VD)

Just curious why do you prefer `D = D | DependencyFlags::Type;` over `D |= 
DependencyFlags::Type;` ? The latter seems to be more common.



Comment at: clang/include/clang/AST/Stmt.h:323
   };
   enum { NumExprBits = NumStmtBits + 9 };
 

Please use `enum { NumExprBits = NumStmtBits + 5 +  DependencyFlagsBits };` to 
avoid bugs when the size changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

Re: [clang] 3ced239 - Refactor CompareReferenceRelationship and its callers in preparation for

[Kim Gräsman via cfe-commits]

Hi Richard,

I see the commit message mentions type sugar here; does this change
affect the AST at all?

We're seeing test failures in IWYU based on recent Clang, and I'm
suspecting this commit (it takes me a while to bisect because of Clang
build times on my laptop).

Thanks,
- Kim

On Wed, Dec 18, 2019 at 11:06 PM Richard Smith via cfe-commits
 wrote:
>
>
> Author: Richard Smith
> Date: 2019-12-18T14:05:57-08:00
> New Revision: 3ced23976aa8a86a17017c87821c873b4ca80bc2
>
> URL: 
> https://github.com/llvm/llvm-project/commit/3ced23976aa8a86a17017c87821c873b4ca80bc2
> DIFF: 
> https://github.com/llvm/llvm-project/commit/3ced23976aa8a86a17017c87821c873b4ca80bc2.diff
>
> LOG: Refactor CompareReferenceRelationship and its callers in preparation for
> implementing the resolution of CWG2352.
>
> No functionality change, except that we now convert the referent of a
> reference binding to the underlying type of the reference in more cases;
> we used to happen to preserve the type sugar from the referent if the
> only type change was in the cv-qualifiers.
>
> This exposed a bug in how we generate code for trivial assignment
> operators: if the type sugar (particularly the may_alias attribute)
> got lost during reference binding, we'd use the "wrong" TBAA information
> for the load during the assignment.
>
> Added:
>
>
> Modified:
> clang/include/clang/Sema/Sema.h
> clang/lib/CodeGen/CGExprCXX.cpp
> clang/lib/Sema/SemaCast.cpp
> clang/lib/Sema/SemaExprCXX.cpp
> clang/lib/Sema/SemaInit.cpp
> clang/lib/Sema/SemaOverload.cpp
> clang/test/AST/ast-dump-expr-json.cpp
> clang/test/Index/print-type.cpp
>
> Removed:
>
>
>
> 
> diff  --git a/clang/include/clang/Sema/Sema.h 
> b/clang/include/clang/Sema/Sema.h
> index 2730eef0bdd8..07eba0306c98 100755
> --- a/clang/include/clang/Sema/Sema.h
> +++ b/clang/include/clang/Sema/Sema.h
> @@ -31,6 +31,7 @@
>  #include "clang/AST/StmtCXX.h"
>  #include "clang/AST/TypeLoc.h"
>  #include "clang/AST/TypeOrdering.h"
> +#include "clang/Basic/BitmaskEnum.h"
>  #include "clang/Basic/ExpressionTraits.h"
>  #include "clang/Basic/Module.h"
>  #include "clang/Basic/OpenMPKinds.h"
> @@ -10703,11 +10704,26 @@ class Sema final {
>  Ref_Compatible
>};
>
> +  // Fake up a scoped enumeration that still contextually converts to bool.
> +  struct ReferenceConversionsScope {
> +/// The conversions that would be performed on an lvalue of type T2 when
> +/// binding a reference of type T1 to it, as determined when evaluating
> +/// whether T1 is reference-compatible with T2.
> +enum ReferenceConversions {
> +  Qualification = 0x1,
> +  Function = 0x2,
> +  DerivedToBase = 0x4,
> +  ObjC = 0x8,
> +  ObjCLifetime = 0x10,
> +
> +  LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/ObjCLifetime)
> +};
> +  };
> +  using ReferenceConversions = 
> ReferenceConversionsScope::ReferenceConversions;
> +
>ReferenceCompareResult
>CompareReferenceRelationship(SourceLocation Loc, QualType T1, QualType T2,
> -   bool &DerivedToBase, bool &ObjCConversion,
> -   bool &ObjCLifetimeConversion,
> -   bool &FunctionConversion);
> +   ReferenceConversions *Conv = nullptr);
>
>ExprResult checkUnknownAnyCast(SourceRange TypeRange, QualType CastType,
>   Expr *CastExpr, CastKind &CastKind,
>
> diff  --git a/clang/lib/CodeGen/CGExprCXX.cpp 
> b/clang/lib/CodeGen/CGExprCXX.cpp
> index 269b80b43403..3fc86136c529 100644
> --- a/clang/lib/CodeGen/CGExprCXX.cpp
> +++ b/clang/lib/CodeGen/CGExprCXX.cpp
> @@ -241,16 +241,28 @@ RValue 
> CodeGenFunction::EmitCXXMemberOrOperatorMemberCallExpr(
>  }
>}
>
> +  bool TrivialForCodegen =
> +  MD->isTrivial() || (MD->isDefaulted() && MD->getParent()->isUnion());
> +  bool TrivialAssignment =
> +  TrivialForCodegen &&
> +  (MD->isCopyAssignmentOperator() || MD->isMoveAssignmentOperator()) &&
> +  !MD->getParent()->mayInsertExtraPadding();
> +
>// C++17 demands that we evaluate the RHS of a (possibly-compound) 
> assignment
>// operator before the LHS.
>CallArgList RtlArgStorage;
>CallArgList *RtlArgs = nullptr;
> +  LValue TrivialAssignmentRHS;
>if (auto *OCE = dyn_cast(CE)) {
>  if (OCE->isAssignmentOp()) {
> -  RtlArgs = &RtlArgStorage;
> -  EmitCallArgs(*RtlArgs, MD->getType()->castAs(),
> -   drop_begin(CE->arguments(), 1), CE->getDirectCallee(),
> -   /*ParamsToSkip*/0, EvaluationOrder::ForceRightToLeft);
> +  if (TrivialAssignment) {
> +TrivialAssignmentRHS = EmitLValue(CE->getArg(1));
> +  } else {
> +RtlArgs = &RtlArgStorage;
> +EmitCallArgs(*RtlArgs, MD->getType()->castAs(),
> + drop_begin(CE->arguments(), 1), CE->getDirectCallee(),
> +  

[PATCH] D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal marked an inline comment as done.
vingeldal added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:123-124
+   "member variable %0 provides global access to non-const type, "
+   "consider "
+   "making the referenced data const")
+  << MemberReference; // FIXME: Add fix-it hint to MemberReference

aaron.ballman wrote:
> vingeldal wrote:
> > aaron.ballman wrote:
> > > Can re-flow this string literal.
> > Sorry, I don't understand what you mean by re-flow.
> Sorry for not being clear -- the string literal spans three lines when it 
> only needs to span two by re-writing the literal. e.g.,
> ```
> "member variable %0 provides global access to non-const type, "
> "consider making the pointed-to data const"
> ```
Ah, yes that's looks better of course. Will fix that in coming update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70265



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

[PATCH] D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal marked an inline comment as done.
vingeldal added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:29
+  unless(anyOf(
+  isConstexpr(), hasDeclContext(namespaceDecl(isAnonymous())),
+  hasType(isConstQualified()),

aaron.ballman wrote:
> vingeldal wrote:
> > aaron.ballman wrote:
> > > Why are you filtering out anonymous namespaces?
> > If it's in an anonymous namespace it's no longer globally accessible, it's 
> > only accessible within the file it is declared.
> It is, however, at namespace scope which is what the C++ Core Guideline 
> suggests to diagnose. Do you have evidence that this is the interpretation 
> the guideline authors were looking for (perhaps they would like to update 
> their suggested guidance for diagnosing)?
> 
> There are two dependency issues to global variables -- one is surprising 
> linkage interactions (where the extern nature of the symbol is an issue 
> across module boundaries) and the other is surprising name lookup behavior 
> within the TU (where the global nature of the symbol is an issue). e.g.,
> ```
> namespace {
> int ik;
> }
> 
> void f() {
>   for (int ij = 0; ik < 10; ij++) { // Oops, typo, but still compiles
>   }
> }
> ```
> That's why I think the guideline purposefully does not exclude things like 
> anonymous namespaces.
I don't have any evidence. To me the guideline still looks a bit draft-like, so 
I just tried my best guess as to the intent of the guideline.
In reading the guideline I get the impression that the intent is to avoid 
globally accessible data which is mutable,
mainly for two reason:
 * It's hard to reason about code where you are dependent upon state which can 
be changed from anywhere in the code.
 * There is a risk of race conditions with this kind of data.

Keeping the variable in an anonymous namespace seems to deals with the issues 
which I interpret to be the focus of this guideline.
Consider that the guideline is part of the interface section. If the variable 
is declared in an anonymous namespace there is no risk that a user of some 
service interface adds a dependency to that variable, since the variable will 
be a hidden part of the provider implementation.

Admittedly the wording doesn't mention an exception for anonymous namespaces, 
and the sentence you have referenced seems to suggest that any non-const 
variable in namespace scope should be matched.
I'm guessing though, that was intended to clarify that a variable is still 
global even if in a (named) namespace, because that would not have been an 
obvious interpretation otherwise.
Without the referenced sentence one might interpret only variables outside of 
namespace scope as global.
Arguably a variable in namespace scope isn't globally declared, though it is 
globally accessible, via the namespace name. I think the global accessibility 
is the reason for dragging in variables from namespace scope and if that is the 
case then we shouldn't also need to worry about anonymous namespaces.
This should probably be clarified in the actual guideline.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70265



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

[PATCH] D71962: Fix crash in getFullyQualifiedName for inline namespace

[Alexey Bader via Phabricator via cfe-commits]

bader created this revision.
bader added reviewers: bkramer, ilya-biryukov.
Herald added subscribers: cfe-commits, ebevhan.
Herald added a project: clang.

The ICE happens when the most outer namespace is an inline namespace.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71962

Files:
  clang/lib/AST/QualTypeNames.cpp
  clang/unittests/Tooling/QualTypeNamesTest.cpp


Index: clang/unittests/Tooling/QualTypeNamesTest.cpp
===
--- clang/unittests/Tooling/QualTypeNamesTest.cpp
+++ clang/unittests/Tooling/QualTypeNamesTest.cpp
@@ -223,6 +223,17 @@
   "}\n"
   );
 
+  TypeNameVisitor InlineNamespace;
+  InlineNamespace.ExpectedQualTypeNames["c"] = "B::C";
+  InlineNamespace.runOver("inline namespace A {\n"
+  "  namespace B {\n"
+  "class C {};\n"
+  "  }\n"
+  "}\n"
+  "using namespace A::B;\n"
+  "C c;\n",
+  TypeNameVisitor::Lang_CXX11);
+
   TypeNameVisitor AnonStrucs;
   AnonStrucs.ExpectedQualTypeNames["a"] = "short";
   AnonStrucs.ExpectedQualTypeNames["un_in_st_1"] =
Index: clang/lib/AST/QualTypeNames.cpp
===
--- clang/lib/AST/QualTypeNames.cpp
+++ clang/lib/AST/QualTypeNames.cpp
@@ -192,7 +192,7 @@
   // Ignore inline namespace;
   NS = dyn_cast(NS->getDeclContext());
 }
-if (NS->getDeclName()) {
+if (NS && NS->getDeclName()) {
   return createNestedNameSpecifier(Ctx, NS, WithGlobalNsPrefix);
 }
 return nullptr;  // no starting '::', no anonymous


Index: clang/unittests/Tooling/QualTypeNamesTest.cpp
===
--- clang/unittests/Tooling/QualTypeNamesTest.cpp
+++ clang/unittests/Tooling/QualTypeNamesTest.cpp
@@ -223,6 +223,17 @@
   "}\n"
   );
 
+  TypeNameVisitor InlineNamespace;
+  InlineNamespace.ExpectedQualTypeNames["c"] = "B::C";
+  InlineNamespace.runOver("inline namespace A {\n"
+  "  namespace B {\n"
+  "class C {};\n"
+  "  }\n"
+  "}\n"
+  "using namespace A::B;\n"
+  "C c;\n",
+  TypeNameVisitor::Lang_CXX11);
+
   TypeNameVisitor AnonStrucs;
   AnonStrucs.ExpectedQualTypeNames["a"] = "short";
   AnonStrucs.ExpectedQualTypeNames["un_in_st_1"] =
Index: clang/lib/AST/QualTypeNames.cpp
===
--- clang/lib/AST/QualTypeNames.cpp
+++ clang/lib/AST/QualTypeNames.cpp
@@ -192,7 +192,7 @@
   // Ignore inline namespace;
   NS = dyn_cast(NS->getDeclContext());
 }
-if (NS->getDeclName()) {
+if (NS && NS->getDeclName()) {
   return createNestedNameSpecifier(Ctx, NS, WithGlobalNsPrefix);
 }
 return nullptr;  // no starting '::', no anonymous
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71962: Fix crash in getFullyQualifiedName for inline namespace

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks for fixing this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71962



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

[PATCH] D71962: Fix crash in getFullyQualifiedName for inline namespace

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71962



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

[clang] 128f39d - Fix crash in getFullyQualifiedName for inline namespace

[Alexey Bader via cfe-commits]

Author: Alexey Bader
Date: 2019-12-28T16:35:51+03:00
New Revision: 128f39da932be50cb49646084820119e6e0d1e22

URL: 
https://github.com/llvm/llvm-project/commit/128f39da932be50cb49646084820119e6e0d1e22
DIFF: 
https://github.com/llvm/llvm-project/commit/128f39da932be50cb49646084820119e6e0d1e22.diff

LOG: Fix crash in getFullyQualifiedName for inline namespace

Summary: The ICE happens when the most outer namespace is an inline namespace.

Reviewers: bkramer, ilya-biryukov

Reviewed By: ilya-biryukov

Subscribers: ebevhan, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D71962

Added: 


Modified: 
clang/lib/AST/QualTypeNames.cpp
clang/unittests/Tooling/QualTypeNamesTest.cpp

Removed: 




diff  --git a/clang/lib/AST/QualTypeNames.cpp b/clang/lib/AST/QualTypeNames.cpp
index f28f00171cce..73a33a208233 100644
--- a/clang/lib/AST/QualTypeNames.cpp
+++ b/clang/lib/AST/QualTypeNames.cpp
@@ -192,7 +192,7 @@ static NestedNameSpecifier *createOuterNNS(const ASTContext 
&Ctx, const Decl *D,
   // Ignore inline namespace;
   NS = dyn_cast(NS->getDeclContext());
 }
-if (NS->getDeclName()) {
+if (NS && NS->getDeclName()) {
   return createNestedNameSpecifier(Ctx, NS, WithGlobalNsPrefix);
 }
 return nullptr;  // no starting '::', no anonymous

diff  --git a/clang/unittests/Tooling/QualTypeNamesTest.cpp 
b/clang/unittests/Tooling/QualTypeNamesTest.cpp
index b6c302977876..ff6b78c2666d 100644
--- a/clang/unittests/Tooling/QualTypeNamesTest.cpp
+++ b/clang/unittests/Tooling/QualTypeNamesTest.cpp
@@ -223,6 +223,17 @@ TEST(QualTypeNameTest, getFullyQualifiedName) {
   "}\n"
   );
 
+  TypeNameVisitor InlineNamespace;
+  InlineNamespace.ExpectedQualTypeNames["c"] = "B::C";
+  InlineNamespace.runOver("inline namespace A {\n"
+  "  namespace B {\n"
+  "class C {};\n"
+  "  }\n"
+  "}\n"
+  "using namespace A::B;\n"
+  "C c;\n",
+  TypeNameVisitor::Lang_CXX11);
+
   TypeNameVisitor AnonStrucs;
   AnonStrucs.ExpectedQualTypeNames["a"] = "short";
   AnonStrucs.ExpectedQualTypeNames["un_in_st_1"] =



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

[PATCH] D71962: Fix crash in getFullyQualifiedName for inline namespace

[Alexey Bader via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG128f39da932b: Fix crash in getFullyQualifiedName for inline 
namespace (authored by bader).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71962

Files:
  clang/lib/AST/QualTypeNames.cpp
  clang/unittests/Tooling/QualTypeNamesTest.cpp


Index: clang/unittests/Tooling/QualTypeNamesTest.cpp
===
--- clang/unittests/Tooling/QualTypeNamesTest.cpp
+++ clang/unittests/Tooling/QualTypeNamesTest.cpp
@@ -223,6 +223,17 @@
   "}\n"
   );
 
+  TypeNameVisitor InlineNamespace;
+  InlineNamespace.ExpectedQualTypeNames["c"] = "B::C";
+  InlineNamespace.runOver("inline namespace A {\n"
+  "  namespace B {\n"
+  "class C {};\n"
+  "  }\n"
+  "}\n"
+  "using namespace A::B;\n"
+  "C c;\n",
+  TypeNameVisitor::Lang_CXX11);
+
   TypeNameVisitor AnonStrucs;
   AnonStrucs.ExpectedQualTypeNames["a"] = "short";
   AnonStrucs.ExpectedQualTypeNames["un_in_st_1"] =
Index: clang/lib/AST/QualTypeNames.cpp
===
--- clang/lib/AST/QualTypeNames.cpp
+++ clang/lib/AST/QualTypeNames.cpp
@@ -192,7 +192,7 @@
   // Ignore inline namespace;
   NS = dyn_cast(NS->getDeclContext());
 }
-if (NS->getDeclName()) {
+if (NS && NS->getDeclName()) {
   return createNestedNameSpecifier(Ctx, NS, WithGlobalNsPrefix);
 }
 return nullptr;  // no starting '::', no anonymous


Index: clang/unittests/Tooling/QualTypeNamesTest.cpp
===
--- clang/unittests/Tooling/QualTypeNamesTest.cpp
+++ clang/unittests/Tooling/QualTypeNamesTest.cpp
@@ -223,6 +223,17 @@
   "}\n"
   );
 
+  TypeNameVisitor InlineNamespace;
+  InlineNamespace.ExpectedQualTypeNames["c"] = "B::C";
+  InlineNamespace.runOver("inline namespace A {\n"
+  "  namespace B {\n"
+  "class C {};\n"
+  "  }\n"
+  "}\n"
+  "using namespace A::B;\n"
+  "C c;\n",
+  TypeNameVisitor::Lang_CXX11);
+
   TypeNameVisitor AnonStrucs;
   AnonStrucs.ExpectedQualTypeNames["a"] = "short";
   AnonStrucs.ExpectedQualTypeNames["un_in_st_1"] =
Index: clang/lib/AST/QualTypeNames.cpp
===
--- clang/lib/AST/QualTypeNames.cpp
+++ clang/lib/AST/QualTypeNames.cpp
@@ -192,7 +192,7 @@
   // Ignore inline namespace;
   NS = dyn_cast(NS->getDeclContext());
 }
-if (NS->getDeclName()) {
+if (NS && NS->getDeclName()) {
   return createNestedNameSpecifier(Ctx, NS, WithGlobalNsPrefix);
 }
 return nullptr;  // no starting '::', no anonymous
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71962: Fix crash in getFullyQualifiedName for inline namespace

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61132 tests passed, 0 failed 
and 728 were skipped.

{icon check-circle color=green} clang-tidy: pass.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71962



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

[PATCH] D71963: clang-tidy doc: Add the severities description

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru created this revision.
sylvestre.ledru added reviewers: dkrupp, aaron.ballman.
Herald added subscribers: rnkovacs, whisperity.
Herald added a project: clang.

I took the text from codechecker. Daniel Krupp ( @dkrupp ) wrote it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71963

Files:
  clang-tools-extra/docs/clang-tidy/checks/list.rst


Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -403,6 +403,26 @@
`hicpp-use-override `_, `modernize-use-override 
`_, "Yes", "low"
`hicpp-vararg `_, `cppcoreguidelines-pro-type-vararg 
`_, , "low"
 
+Severities
+--
+
+.. Severities description are coming from Codechecker too:
+   https://github.com/Ericsson/codechecker/blob/master/config/config.md
+
+The following severity levels are defined:
+
+- **STYLE**: A true positive indicates that the source code is against a 
specific coding guideline or could improve readability.
+  Example: LLVM Coding Guideline: Do not use else or else if after something 
that interrupts control flow (break, return, throw, continue).
+
+- **LOW**: A true positive indicates that the source code is hard to 
read/understand or could be easily optimized.
+  Example: Unused variables, Dead code.
+
+- **MEDIUM**: A true positive indicates that the source code that may not 
cause a run-time error (yet), but against intuition and hence prone to error.
+  Example: Redundant expression in a condition.
+
+- **HIGH**: A true positive indicates that the source code will cause a 
run-time error.
+  Example of this category: out of bounds array access, division by zero, 
memory leak.
+
 .. toctree::
:glob:
:hidden:


Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -403,6 +403,26 @@
`hicpp-use-override `_, `modernize-use-override `_, "Yes", "low"
`hicpp-vararg `_, `cppcoreguidelines-pro-type-vararg `_, , "low"
 
+Severities
+--
+
+.. Severities description are coming from Codechecker too:
+   https://github.com/Ericsson/codechecker/blob/master/config/config.md
+
+The following severity levels are defined:
+
+- **STYLE**: A true positive indicates that the source code is against a specific coding guideline or could improve readability.
+  Example: LLVM Coding Guideline: Do not use else or else if after something that interrupts control flow (break, return, throw, continue).
+
+- **LOW**: A true positive indicates that the source code is hard to read/understand or could be easily optimized.
+  Example: Unused variables, Dead code.
+
+- **MEDIUM**: A true positive indicates that the source code that may not cause a run-time error (yet), but against intuition and hence prone to error.
+  Example: Redundant expression in a condition.
+
+- **HIGH**: A true positive indicates that the source code will cause a run-time error.
+  Example of this category: out of bounds array access, division by zero, memory leak.
+
 .. toctree::
:glob:
:hidden:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added inline comments.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:198
cppcoreguidelines-avoid-magic-numbers (redirects to 
readability-magic-numbers) 
+   cppcoreguidelines-avoid-non-const-global-variables
cppcoreguidelines-c-copy-assignment-signature (redirects to 
misc-unconventional-assign-operator) 


aaron.ballman wrote:
> vingeldal wrote:
> > sylvestre.ledru wrote:
> > > list.rst changed, you should update this!
> > > Thanks
> > > 
> > I'm a bit concerned with this previous change of of list.rst.
> > 
> > Now that I need to add a check to this file, I don't know what severity to 
> > give it. I can't find any definition of severity levels and I really want 
> > to avoid getting into a long discussion, or having different reviewers not 
> > agreeing on my patch, concerning what severity we should give this check.
> > Is there any way I can find some kind of guideline on how to set the 
> > severity to avoid an opinionated discussion about severity level?
> I'd like to follow that discussion so that I can be consistent with my review 
> advice, too. 
Good point!

I propose this patch to explain
https://reviews.llvm.org/D71963

this is a lazy copy and paste of 
https://github.com/Ericsson/codechecker/blob/master/config/config.md



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70265



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

[PATCH] D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added inline comments.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:411
*
+   cppcoreguidelines-avoid-non-const-global-variables
+

It should not be there. It won't be referenced on the list.

If you are uncomfortable setting a severity, leave it empty but it should 
clearly move above, not on the toctree 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70265



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

[PATCH] D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal updated this revision to Diff 235483.
vingeldal added a comment.

Updating D70265 : [clang-tidy] Add clang tidy 
check I.2 to cppcoreguidelines

- Adjusted the line breaks of some string literals.
- Modified list.rst


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70265

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables-IgnoreDataMembers.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
@@ -0,0 +1,268 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-non-const-global-variables %t
+
+int nonConstInt = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: variable 'nonConstInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+
+int &nonConstIntReference = nonConstInt;
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: variable 'nonConstIntReference' provides global access to non-const type, consider making the referenced data const [cppcoreguidelines-avoid-non-const-global-variables]
+
+int *pointerToNonConstInt = &nonConstInt;
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: variable 'pointerToNonConstInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES: :[[@LINE-2]]:6: warning: variable 'pointerToNonConstInt' provides global access to non-const type, consider making the pointed-to data const [cppcoreguidelines-avoid-non-const-global-variables]
+
+int *const constPointerToNonConstInt = &nonConstInt;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'constPointerToNonConstInt' provides global access to non-const type, consider making the pointed-to data const [cppcoreguidelines-avoid-non-const-global-variables]
+
+namespace namespace_name {
+int nonConstNamespaceInt = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: variable 'nonConstNamespaceInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+
+const int constNamespaceInt = 0;
+} // namespace namespace_name
+
+const int constInt = 0;
+
+const int *pointerToConstInt = &constInt;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'pointerToConstInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+
+const int *const constPointerToConstInt = &constInt;
+
+const int &constReferenceToConstInt = constInt;
+
+constexpr int constexprInt = 0;
+
+int function() {
+  int nonConstReturnValue = 0;
+  return nonConstReturnValue;
+}
+
+namespace {
+int nonConstAnonymousNamespaceInt = 0;
+}
+
+class DummyClass {
+public:
+  int nonConstMemberVariable = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: member variable 'nonConstMemberVariable' is globally accessible and non-const, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+  const int constMemberVariable = 0;
+  int *const constMemberPointerToNonConst = &nonConstInt;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member variable 'constMemberPointerToNonConst' provides global access to non-const type, consider making the pointed-to data const [cppcoreguidelines-avoid-non-const-global-variables]
+  int *memberPointerToNonConst = &nonConstInt;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member variable 'memberPointerToNonConst' is globally accessible and non-const, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+  // CHECK-MESSAGES: :[[@LINE-2]]:8: warning: member variable 'memberPointerToNonConst' provides global access to non-const type, consider making the pointed-to data const [cppcoreguidelines-avoid-non-const-global-variables]
+  const int *const constMemberPointerToConst = &constInt;
+  int &nonConstMemberReferenceToNonConst = nonConstInt;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member variable 'nonConstMemberReferenceToNonConst' is globally accessible and non-const, consider making it const [cppcoreguidelines-avoid-non-const-global-variab

[PATCH] D71939: clang-format: fix conflict between FormatStyle::BWACS_MultiLine and BeforeCatch/BeforeElse

[Pavlo Shkrabliuk via Phabricator via cfe-commits]

pastey added a comment.

Thanks for your feedback. Let's see what other reviewers say.


Repository:
  rC Clang

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

https://reviews.llvm.org/D71939



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

[PATCH] D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal marked 2 inline comments as done.
vingeldal added inline comments.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:198
cppcoreguidelines-avoid-magic-numbers (redirects to 
readability-magic-numbers) 
+   cppcoreguidelines-avoid-non-const-global-variables
cppcoreguidelines-c-copy-assignment-signature (redirects to 
misc-unconventional-assign-operator) 


sylvestre.ledru wrote:
> aaron.ballman wrote:
> > vingeldal wrote:
> > > sylvestre.ledru wrote:
> > > > list.rst changed, you should update this!
> > > > Thanks
> > > > 
> > > I'm a bit concerned with this previous change of of list.rst.
> > > 
> > > Now that I need to add a check to this file, I don't know what severity 
> > > to give it. I can't find any definition of severity levels and I really 
> > > want to avoid getting into a long discussion, or having different 
> > > reviewers not agreeing on my patch, concerning what severity we should 
> > > give this check.
> > > Is there any way I can find some kind of guideline on how to set the 
> > > severity to avoid an opinionated discussion about severity level?
> > I'd like to follow that discussion so that I can be consistent with my 
> > review advice, too. 
> Good point!
> 
> I propose this patch to explain
> https://reviews.llvm.org/D71963
> 
> this is a lazy copy and paste of 
> https://github.com/Ericsson/codechecker/blob/master/config/config.md
> 
Great, will make sure to contribute some feedback to that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70265



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

Re: [clang] 3ced239 - Refactor CompareReferenceRelationship and its callers in preparation for

[Kim Gräsman via cfe-commits]

Yes, it is this change that broke us.

I still don't fully understand what the change was, but it appears to
mess things up for IWYU for operator== with templates and const,
somehow.

Could you expand on what changed here and how the AST might be affected?

Thanks,
- Kim

On Sat, Dec 28, 2019 at 4:35 PM Kim Gräsman  wrote:
>
> Hi Richard,
>
> I see the commit message mentions type sugar here; does this change
> affect the AST at all?
>
> We're seeing test failures in IWYU based on recent Clang, and I'm
> suspecting this commit (it takes me a while to bisect because of Clang
> build times on my laptop).
>
> Thanks,
> - Kim
>
> On Wed, Dec 18, 2019 at 11:06 PM Richard Smith via cfe-commits
>  wrote:
> >
> >
> > Author: Richard Smith
> > Date: 2019-12-18T14:05:57-08:00
> > New Revision: 3ced23976aa8a86a17017c87821c873b4ca80bc2
> >
> > URL: 
> > https://github.com/llvm/llvm-project/commit/3ced23976aa8a86a17017c87821c873b4ca80bc2
> > DIFF: 
> > https://github.com/llvm/llvm-project/commit/3ced23976aa8a86a17017c87821c873b4ca80bc2.diff
> >
> > LOG: Refactor CompareReferenceRelationship and its callers in preparation 
> > for
> > implementing the resolution of CWG2352.
> >
> > No functionality change, except that we now convert the referent of a
> > reference binding to the underlying type of the reference in more cases;
> > we used to happen to preserve the type sugar from the referent if the
> > only type change was in the cv-qualifiers.
> >
> > This exposed a bug in how we generate code for trivial assignment
> > operators: if the type sugar (particularly the may_alias attribute)
> > got lost during reference binding, we'd use the "wrong" TBAA information
> > for the load during the assignment.
> >
> > Added:
> >
> >
> > Modified:
> > clang/include/clang/Sema/Sema.h
> > clang/lib/CodeGen/CGExprCXX.cpp
> > clang/lib/Sema/SemaCast.cpp
> > clang/lib/Sema/SemaExprCXX.cpp
> > clang/lib/Sema/SemaInit.cpp
> > clang/lib/Sema/SemaOverload.cpp
> > clang/test/AST/ast-dump-expr-json.cpp
> > clang/test/Index/print-type.cpp
> >
> > Removed:
> >
> >
> >
> > 
> > diff  --git a/clang/include/clang/Sema/Sema.h 
> > b/clang/include/clang/Sema/Sema.h
> > index 2730eef0bdd8..07eba0306c98 100755
> > --- a/clang/include/clang/Sema/Sema.h
> > +++ b/clang/include/clang/Sema/Sema.h
> > @@ -31,6 +31,7 @@
> >  #include "clang/AST/StmtCXX.h"
> >  #include "clang/AST/TypeLoc.h"
> >  #include "clang/AST/TypeOrdering.h"
> > +#include "clang/Basic/BitmaskEnum.h"
> >  #include "clang/Basic/ExpressionTraits.h"
> >  #include "clang/Basic/Module.h"
> >  #include "clang/Basic/OpenMPKinds.h"
> > @@ -10703,11 +10704,26 @@ class Sema final {
> >  Ref_Compatible
> >};
> >
> > +  // Fake up a scoped enumeration that still contextually converts to bool.
> > +  struct ReferenceConversionsScope {
> > +/// The conversions that would be performed on an lvalue of type T2 
> > when
> > +/// binding a reference of type T1 to it, as determined when evaluating
> > +/// whether T1 is reference-compatible with T2.
> > +enum ReferenceConversions {
> > +  Qualification = 0x1,
> > +  Function = 0x2,
> > +  DerivedToBase = 0x4,
> > +  ObjC = 0x8,
> > +  ObjCLifetime = 0x10,
> > +
> > +  LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/ObjCLifetime)
> > +};
> > +  };
> > +  using ReferenceConversions = 
> > ReferenceConversionsScope::ReferenceConversions;
> > +
> >ReferenceCompareResult
> >CompareReferenceRelationship(SourceLocation Loc, QualType T1, QualType 
> > T2,
> > -   bool &DerivedToBase, bool &ObjCConversion,
> > -   bool &ObjCLifetimeConversion,
> > -   bool &FunctionConversion);
> > +   ReferenceConversions *Conv = nullptr);
> >
> >ExprResult checkUnknownAnyCast(SourceRange TypeRange, QualType CastType,
> >   Expr *CastExpr, CastKind &CastKind,
> >
> > diff  --git a/clang/lib/CodeGen/CGExprCXX.cpp 
> > b/clang/lib/CodeGen/CGExprCXX.cpp
> > index 269b80b43403..3fc86136c529 100644
> > --- a/clang/lib/CodeGen/CGExprCXX.cpp
> > +++ b/clang/lib/CodeGen/CGExprCXX.cpp
> > @@ -241,16 +241,28 @@ RValue 
> > CodeGenFunction::EmitCXXMemberOrOperatorMemberCallExpr(
> >  }
> >}
> >
> > +  bool TrivialForCodegen =
> > +  MD->isTrivial() || (MD->isDefaulted() && MD->getParent()->isUnion());
> > +  bool TrivialAssignment =
> > +  TrivialForCodegen &&
> > +  (MD->isCopyAssignmentOperator() || MD->isMoveAssignmentOperator()) &&
> > +  !MD->getParent()->mayInsertExtraPadding();
> > +
> >// C++17 demands that we evaluate the RHS of a (possibly-compound) 
> > assignment
> >// operator before the LHS.
> >CallArgList RtlArgStorage;
> >CallArgList *RtlArgs = nullptr;
> > +  LValue TrivialAssignmentRHS;
> >if (auto *OCE = dyn_cast(CE

[PATCH] D71965: include missing for std::abort

[Khem Raj via Phabricator via cfe-commits]

raj.khem created this revision.
raj.khem added reviewers: kadircet, jfb.
raj.khem added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, usaxena95, arphaman, dexonsmith, 
jkorous, ilya-biryukov.
Herald added a project: clang.

Fixes

TOPDIR/build/tmp/work-shared/llvm-project-source-10.0.0-r0/git/clang-tools-extra/clangd/Shutdown.cpp:21:10:
 error: no member named 'abort' in namespace 'std'

  std::abort();
  ~^

TOPDIR/build/tmp/work-shared/llvm-project-source-10.0.0-r0/git/clang-tools-extra/clangd/Shutdown.cpp:30:10:
 error: no member named 'abort' in namespace 'std'

  std::abort();
  ~^


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71965

Files:
  clang-tools-extra/clangd/Shutdown.cpp


Index: clang-tools-extra/clangd/Shutdown.cpp
===
--- clang-tools-extra/clangd/Shutdown.cpp
+++ clang-tools-extra/clangd/Shutdown.cpp
@@ -9,6 +9,7 @@
 #include "Shutdown.h"
 
 #include 
+#include 
 #include 
 
 namespace clang {


Index: clang-tools-extra/clangd/Shutdown.cpp
===
--- clang-tools-extra/clangd/Shutdown.cpp
+++ clang-tools-extra/clangd/Shutdown.cpp
@@ -9,6 +9,7 @@
 #include "Shutdown.h"
 
 #include 
+#include 
 #include 
 
 namespace clang {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71965: include missing for std::abort

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61133 tests passed, 0 failed 
and 728 were skipped.

{icon check-circle color=green} clang-tidy: pass.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71965



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

[PATCH] D71966: [Wdocumentation][RFC] Improve identifier's of \param

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon question-circle color=gray} Unit tests: unknown.

{icon times-circle color=red} clang-tidy: fail. Please fix clang-tidy findings 
.

{icon times-circle color=red} clang-format: fail. Please format your changes 
with clang-format by running `git-clang-format HEAD^` or applying this patch 
.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71966



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

[PATCH] D71966: [Wdocumentation][RFC] Improve identifier's of \param

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

The merge bot noticed I forgot to update clang's unittests. Will look at that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71966



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

[PATCH] D71963: clang-tidy doc: Add the severities description

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I think this is a reasonable first-pass at the severity descriptions, but do 
you think we should add some words that tell the user to use whatever severity 
is picked by a coding standard if one is being followed? For instance, the CERT 
rules all come with a severity specified by the rule itself. I don't see a good 
reason for us to pick a different severity than what is listed by the rule 
author, except if each coding standard has drastically different ideas about 
severity (which I don't think is the case from what I can tell).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71963



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

[PATCH] D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:29
+  unless(anyOf(
+  isConstexpr(), hasDeclContext(namespaceDecl(isAnonymous())),
+  hasType(isConstQualified()),

vingeldal wrote:
> aaron.ballman wrote:
> > vingeldal wrote:
> > > aaron.ballman wrote:
> > > > Why are you filtering out anonymous namespaces?
> > > If it's in an anonymous namespace it's no longer globally accessible, 
> > > it's only accessible within the file it is declared.
> > It is, however, at namespace scope which is what the C++ Core Guideline 
> > suggests to diagnose. Do you have evidence that this is the interpretation 
> > the guideline authors were looking for (perhaps they would like to update 
> > their suggested guidance for diagnosing)?
> > 
> > There are two dependency issues to global variables -- one is surprising 
> > linkage interactions (where the extern nature of the symbol is an issue 
> > across module boundaries) and the other is surprising name lookup behavior 
> > within the TU (where the global nature of the symbol is an issue). e.g.,
> > ```
> > namespace {
> > int ik;
> > }
> > 
> > void f() {
> >   for (int ij = 0; ik < 10; ij++) { // Oops, typo, but still compiles
> >   }
> > }
> > ```
> > That's why I think the guideline purposefully does not exclude things like 
> > anonymous namespaces.
> I don't have any evidence. To me the guideline still looks a bit draft-like, 
> so I just tried my best guess as to the intent of the guideline.
> In reading the guideline I get the impression that the intent is to avoid 
> globally accessible data which is mutable,
> mainly for two reason:
>  * It's hard to reason about code where you are dependent upon state which 
> can be changed from anywhere in the code.
>  * There is a risk of race conditions with this kind of data.
> 
> Keeping the variable in an anonymous namespace seems to deals with the issues 
> which I interpret to be the focus of this guideline.
> Consider that the guideline is part of the interface section. If the variable 
> is declared in an anonymous namespace there is no risk that a user of some 
> service interface adds a dependency to that variable, since the variable will 
> be a hidden part of the provider implementation.
> 
> Admittedly the wording doesn't mention an exception for anonymous namespaces, 
> and the sentence you have referenced seems to suggest that any non-const 
> variable in namespace scope should be matched.
> I'm guessing though, that was intended to clarify that a variable is still 
> global even if in a (named) namespace, because that would not have been an 
> obvious interpretation otherwise.
> Without the referenced sentence one might interpret only variables outside of 
> namespace scope as global.
> Arguably a variable in namespace scope isn't globally declared, though it is 
> globally accessible, via the namespace name. I think the global accessibility 
> is the reason for dragging in variables from namespace scope and if that is 
> the case then we shouldn't also need to worry about anonymous namespaces.
> This should probably be clarified in the actual guideline.
> This should probably be clarified in the actual guideline.

This sort of thing comes up with coding guidelines from time to time and the 
way we usually handle it is to ask the guideline authors for guidance. If they 
come back with an answer, then we go that route (while the authors fix the 
text) and if they don't come back with an answer, we muddle our way through. 
Would you mind filing an issue with the C++ Core Guideline folks to see if they 
can weigh in on the topic? If they don't respond in a timely fashion, I think 
it would make more sense to err on the side of caution and diagnose 
declarations within anonymous namespaces. This matches the current text from 
the core guideline, and we can always relax the rule if we find it causes a lot 
of heartache in the wild. (If you have data about how often this particular 
aspect of the check triggers on large real-world code bases, that could help us 
make a decision too.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70265



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

[PATCH] D71436: [X86][AsmParser] re-introduce 'offset' operator

[Reid Kleckner via Phabricator via cfe-commits]

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

lgtm, thanks!




Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:1821
+getParser().parsePrimaryExpr(Val, End))
+  return Error(Start, "unexpected token!");
+  } else if (ParseIntelInlineAsmIdentifier(Val, ID, Info, false, End, true)) {

epastor wrote:
> epastor wrote:
> > rnk wrote:
> > > Please test this corner case, I imagine it looks like:
> > >   mov eax, offset 3
> > Interesting. This corner case didn't trigger in that scenario; we get an 
> > "expected identifier" error message with good source location, followed by 
> > another error "use of undeclared label '3'" in debug builds... and in 
> > release builds, we instead get a crash. On tracing the crash, it's a 
> > AsmStrRewrite applying to a SMLoc not coming from the same string...
> > 
> > As near as I can tell, the issue is that we end up trying to parse "3" as a 
> > not-yet-declared label, as such expand it to `__MSASMLABEL_.${:uid}__3`, 
> > and then end up in a bad state because the operand rewrite is applying to 
> > the expanded symbol... which isn't in the same AsmString. If you use an 
> > actual undeclared label, you hit the same crash in release builds.
> > 
> > This is going to take some work; I'll get back to it in a day or two.
> Fixed; we now get the same errors in this scenario as we do in the current 
> LLVM trunk, reporting "expected identifier" and "use of undeclared label '3'".
> 
> On the other hand, I'm still working on finding a scenario that DOES trigger 
> this corner case.
I see. Well, it sounds like it was a useful test. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71436



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

[clang] 0acfc49 - Allow redeclaration of __declspec(uuid)

[Reid Kleckner via cfe-commits]

Author: Zachary Henkel
Date: 2019-12-28T13:13:46-08:00
New Revision: 0acfc493171a880215327ecd70e3e01e1cd493df

URL: 
https://github.com/llvm/llvm-project/commit/0acfc493171a880215327ecd70e3e01e1cd493df
DIFF: 
https://github.com/llvm/llvm-project/commit/0acfc493171a880215327ecd70e3e01e1cd493df.diff

LOG: Allow redeclaration of __declspec(uuid)

msvc allows a subsequent declaration of a uuid attribute on a
struct/class.  Mirror this behavior in clang-cl.

Reviewed By: rnk

Differential Revision: https://reviews.llvm.org/D71439

Added: 


Modified: 
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaDeclAttr.cpp
clang/test/SemaCXX/ms-uuid.cpp

Removed: 




diff  --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 9ca7a80d3ada..92b115c8a3f3 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -2685,6 +2685,10 @@ static void checkNewAttributesAfterDef(Sema &S, Decl 
*New, const Decl *Old) {
   // C's _Noreturn is allowed to be added to a function after it is 
defined.
   ++I;
   continue;
+} else if (isa(NewAttribute)) {
+  // msvc will allow a subsequent definition to add an uuid to a class
+  ++I;
+  continue;
 } else if (const AlignedAttr *AA = dyn_cast(NewAttribute)) {
   if (AA->isAlignas()) {
 // C++11 [dcl.align]p6:

diff  --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index ac83a9b156e8..50951ad60228 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -5394,9 +5394,11 @@ UuidAttr *Sema::mergeUuidAttr(Decl *D, const 
AttributeCommonInfo &CI,
   if (const auto *UA = D->getAttr()) {
 if (UA->getGuid().equals_lower(Uuid))
   return nullptr;
-Diag(UA->getLocation(), diag::err_mismatched_uuid);
-Diag(CI.getLoc(), diag::note_previous_uuid);
-D->dropAttr();
+if (!UA->getGuid().empty()) {
+  Diag(UA->getLocation(), diag::err_mismatched_uuid);
+  Diag(CI.getLoc(), diag::note_previous_uuid);
+  D->dropAttr();
+}
   }
 
   return ::new (Context) UuidAttr(Context, CI, Uuid);

diff  --git a/clang/test/SemaCXX/ms-uuid.cpp b/clang/test/SemaCXX/ms-uuid.cpp
index c177570682f6..1de7aee90c46 100644
--- a/clang/test/SemaCXX/ms-uuid.cpp
+++ b/clang/test/SemaCXX/ms-uuid.cpp
@@ -106,3 +106,9 @@ void F2() {
 }
 
 }
+
+// Test class/struct redeclaration where the subsequent
+// declaration has a uuid attribute
+struct X{};
+
+struct __declspec(uuid("----")) X;
\ No newline at end of file



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

[PATCH] D71439: Allow redeclaration of __declspec(uuid)

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

In D71439#1789141 , @zahen wrote:

> @rnk Can you please submit on my behalf.  I don't have commit access.


Sorry for the delay, I pushed it today. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71439



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

[PATCH] D71439: Allow redeclaration of __declspec(uuid)

[Reid Kleckner via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0acfc493171a: Allow redeclaration of __declspec(uuid) 
(authored by zahen, committed by rnk).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71439

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/SemaCXX/ms-uuid.cpp


Index: clang/test/SemaCXX/ms-uuid.cpp
===
--- clang/test/SemaCXX/ms-uuid.cpp
+++ clang/test/SemaCXX/ms-uuid.cpp
@@ -106,3 +106,9 @@
 }
 
 }
+
+// Test class/struct redeclaration where the subsequent
+// declaration has a uuid attribute
+struct X{};
+
+struct __declspec(uuid("----")) X;
\ No newline at end of file
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -5394,9 +5394,11 @@
   if (const auto *UA = D->getAttr()) {
 if (UA->getGuid().equals_lower(Uuid))
   return nullptr;
-Diag(UA->getLocation(), diag::err_mismatched_uuid);
-Diag(CI.getLoc(), diag::note_previous_uuid);
-D->dropAttr();
+if (!UA->getGuid().empty()) {
+  Diag(UA->getLocation(), diag::err_mismatched_uuid);
+  Diag(CI.getLoc(), diag::note_previous_uuid);
+  D->dropAttr();
+}
   }
 
   return ::new (Context) UuidAttr(Context, CI, Uuid);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -2685,6 +2685,10 @@
   // C's _Noreturn is allowed to be added to a function after it is 
defined.
   ++I;
   continue;
+} else if (isa(NewAttribute)) {
+  // msvc will allow a subsequent definition to add an uuid to a class
+  ++I;
+  continue;
 } else if (const AlignedAttr *AA = dyn_cast(NewAttribute)) {
   if (AA->isAlignas()) {
 // C++11 [dcl.align]p6:


Index: clang/test/SemaCXX/ms-uuid.cpp
===
--- clang/test/SemaCXX/ms-uuid.cpp
+++ clang/test/SemaCXX/ms-uuid.cpp
@@ -106,3 +106,9 @@
 }
 
 }
+
+// Test class/struct redeclaration where the subsequent
+// declaration has a uuid attribute
+struct X{};
+
+struct __declspec(uuid("----")) X;
\ No newline at end of file
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -5394,9 +5394,11 @@
   if (const auto *UA = D->getAttr()) {
 if (UA->getGuid().equals_lower(Uuid))
   return nullptr;
-Diag(UA->getLocation(), diag::err_mismatched_uuid);
-Diag(CI.getLoc(), diag::note_previous_uuid);
-D->dropAttr();
+if (!UA->getGuid().empty()) {
+  Diag(UA->getLocation(), diag::err_mismatched_uuid);
+  Diag(CI.getLoc(), diag::note_previous_uuid);
+  D->dropAttr();
+}
   }
 
   return ::new (Context) UuidAttr(Context, CI, Uuid);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -2685,6 +2685,10 @@
   // C's _Noreturn is allowed to be added to a function after it is defined.
   ++I;
   continue;
+} else if (isa(NewAttribute)) {
+  // msvc will allow a subsequent definition to add an uuid to a class
+  ++I;
+  continue;
 } else if (const AlignedAttr *AA = dyn_cast(NewAttribute)) {
   if (AA->isAlignas()) {
 // C++11 [dcl.align]p6:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Sorry I didn't make it obvious with the test cases. The fix will never extend 
the lifetime of variables either in the condition or declared in the else 
block. It will only apply if the if is the last statement in its scope. Though 
I should check back through the scope for any declarations that have the same 
identifier as those in the if statement which would cause an error if they were 
brought out of the if scope


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

https://reviews.llvm.org/D71846



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

[PATCH] D71969: [OpenMP] diagnose zero-length array section in the depend clause

[Kelvin Li via Phabricator via cfe-commits]

kkwli0 created this revision.
kkwli0 added a reviewer: ABataev.
Herald added a subscriber: guansong.
Herald added a reviewer: jdoerfert.

The OpenMP specification disallows having zero-length array sections in the 
depend clause (OpenMP 5.0 2.17.11).


https://reviews.llvm.org/D71969

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/target_depend_messages.cpp
  clang/test/OpenMP/target_enter_data_depend_messages.cpp
  clang/test/OpenMP/target_exit_data_depend_messages.cpp
  clang/test/OpenMP/target_parallel_depend_messages.cpp
  clang/test/OpenMP/target_parallel_for_depend_messages.cpp
  clang/test/OpenMP/target_parallel_for_simd_depend_messages.cpp
  clang/test/OpenMP/target_simd_depend_messages.cpp
  clang/test/OpenMP/target_teams_depend_messages.cpp
  clang/test/OpenMP/target_teams_distribute_depend_messages.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_depend_messages.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_depend_messages.cpp
  clang/test/OpenMP/target_teams_distribute_simd_depend_messages.cpp
  clang/test/OpenMP/target_update_depend_messages.cpp
  clang/test/OpenMP/task_depend_messages.cpp

Index: clang/test/OpenMP/task_depend_messages.cpp
===
--- clang/test/OpenMP/task_depend_messages.cpp
+++ clang/test/OpenMP/task_depend_messages.cpp
@@ -45,7 +45,7 @@
   #pragma omp task depend (in : argv[argc: // expected-error {{expected expression}} expected-error {{expected ']'}} expected-error {{expected ')'}} expected-note {{to match this '['}} expected-note {{to match this '('}}
   #pragma omp task depend (in : argv[argc:argc] // expected-error {{expected ')'}} expected-note {{to match this '('}}
   #pragma omp task depend (in : argv[0:-1]) // expected-error {{section length is evaluated to a negative value -1}}
-  #pragma omp task depend (in : argv[-1:0])
+  #pragma omp task depend (in : argv[-1:0]) // expected-error {{zero-length array section is not allowed in 'depend' clause}}
   #pragma omp task depend (in : argv[:]) // expected-error {{section length is unspecified and cannot be inferred because subscripted value is not an array}}
   #pragma omp task depend (in : argv[3:4:1]) // expected-error {{expected ']'}} expected-note {{to match this '['}}
   #pragma omp task depend(in:a[0:1]) // expected-error {{subscripted value is not an array or pointer}}
Index: clang/test/OpenMP/target_update_depend_messages.cpp
===
--- clang/test/OpenMP/target_update_depend_messages.cpp
+++ clang/test/OpenMP/target_update_depend_messages.cpp
@@ -15,7 +15,6 @@
   public:
 int operator[](int index) { return 0; }
 };
-
 template 
 int tmain(T argc, S **argv, R *env[]) {
   vector vec;
@@ -52,7 +51,7 @@
   #pragma omp target update to(z) depend(in : argv[argc: // expected-error {{expected expression}} expected-error {{expected ']'}} expected-error {{expected ')'}} expected-note {{to match this '['}} expected-note {{to match this '('}}
   #pragma omp target update to(z) depend(in : argv[argc:argc] // expected-error {{expected ')'}} expected-note {{to match this '('}}
   #pragma omp target update to(z) depend(in : argv[0:-1]) // expected-error {{section length is evaluated to a negative value -1}}
-  #pragma omp target update to(z) depend(in : argv[-1:0])
+  #pragma omp target update to(z) depend(in : argv[-1:0]) // expected-error {{zero-length array section is not allowed in 'depend' clause}}
   #pragma omp target update to(z) depend(in : argv[:]) // expected-error {{section length is unspecified and cannot be inferred because subscripted value is not an array}}
   #pragma omp target update to(z) depend(in : argv[3:4:1]) // expected-error {{expected ']'}} expected-note {{to match this '['}}
   #pragma omp target update to(z) depend(in:a[0:1]) // expected-error {{subscripted value is not an array or pointer}}
@@ -64,7 +63,6 @@
 
   return 0;
 }
-
 int main(int argc, char **argv, char *env[]) {
   vector vec;
   typedef float V __attribute__((vector_size(16)));
@@ -100,7 +98,7 @@
   #pragma omp target update to(z) depend(in : argv[argc: // expected-error {{expected expression}} expected-error {{expected ']'}} expected-error {{expected ')'}} expected-note {{to match this '['}} expected-note {{to match this '('}}
   #pragma omp target update to(z) depend(in : argv[argc:argc] // expected-error {{expected ')'}} expected-note {{to match this '('}}
   #pragma omp target update to(z) depend(in : argv[0:-1]) // expected-error {{section length is evaluated to a negative value -1}}
-  #pragma omp target update to(z) depend(in : argv[-1:0])
+  #pragma omp target update to(z) depend(in : argv[-1:0]) // expected-error {{zero-length array section is not allowed in 'depend' clause}}
   #pragma omp target update to(z) depend(in : argv[:]) // expected-error {{section length is unspecified and cannot be inferred becau

[PATCH] D71953: [Tooling] Infer driver mode and target for FixedCompilationDatabase

[Hanjiang Yu via Phabricator via cfe-commits]

de1acr0ix updated this revision to Diff 235496.
de1acr0ix added a comment.

Check if argv[0] is an option before inferring - anything starting with dash or 
slash (but without a second slash) will be treated as compiler option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71953

Files:
  clang/lib/Tooling/CompilationDatabase.cpp
  clang/unittests/Tooling/CompilationDatabaseTest.cpp

Index: clang/unittests/Tooling/CompilationDatabaseTest.cpp
===
--- clang/unittests/Tooling/CompilationDatabaseTest.cpp
+++ clang/unittests/Tooling/CompilationDatabaseTest.cpp
@@ -605,6 +605,25 @@
   EXPECT_EQ(2, Argc);
 }
 
+TEST(ParseFixedCompilationDatabase, HandlesPositionalArgsSlash) {
+  // Arguments starting with slash will be stripped without explicitly setting
+  // driver mode to CL.
+  const char *Argv[] = {"1", "2", "--", "/O2", "somefile.cpp"};
+  int Argc = sizeof(Argv) / sizeof(char *);
+  std::string ErrorMsg;
+  std::unique_ptr Database =
+  FixedCompilationDatabase::loadFromCommandLine(Argc, Argv, ErrorMsg);
+  ASSERT_TRUE((bool)Database);
+  ASSERT_TRUE(ErrorMsg.empty());
+  std::vector Result = Database->getCompileCommands("source");
+  ASSERT_EQ(1ul, Result.size());
+  ASSERT_EQ(".", Result[0].Directory);
+  std::vector Expected;
+  ASSERT_THAT(Result[0].CommandLine,
+  ElementsAre(EndsWith("clang-tool"), "source"));
+  EXPECT_EQ(2, Argc);
+}
+
 TEST(ParseFixedCompilationDatabase, HandlesPositionalArgsSyntaxOnly) {
   // Adjust the given command line arguments to ensure that any positional
   // arguments in them are stripped.
@@ -641,6 +660,43 @@
   EXPECT_EQ(2, Argc);
 }
 
+TEST(ParseFixedCompilationDatabase, InferDriverMode) {
+  const char *Argv[] = {"1", "2", "--", "cl.exe", "-nologo", "somefile.cpp"};
+  int Argc = sizeof(Argv) / sizeof(char *);
+  std::string ErrorMsg;
+  std::unique_ptr Database =
+  FixedCompilationDatabase::loadFromCommandLine(Argc, Argv, ErrorMsg);
+  ASSERT_TRUE((bool)Database);
+  ASSERT_TRUE(ErrorMsg.empty());
+  std::vector Result = Database->getCompileCommands("source");
+  ASSERT_EQ(1ul, Result.size());
+  ASSERT_EQ(".", Result[0].Directory);
+  std::vector Expected;
+  ASSERT_THAT(Result[0].CommandLine,
+  ElementsAre(EndsWith("clang-tool"), "--driver-mode=cl", "-nologo",
+  "source"));
+  EXPECT_EQ(2, Argc);
+}
+
+TEST(ParseFixedCompilationDatabase, InferTarget) {
+  const char *Argv[] = {"1", "2", "--", "x86_64-linux-gnu-gcc", "somefile.cpp"};
+  int Argc = sizeof(Argv) / sizeof(char *);
+  std::string ErrorMsg;
+  LLVMInitializeX86TargetInfo();
+  std::unique_ptr Database =
+  FixedCompilationDatabase::loadFromCommandLine(Argc, Argv, ErrorMsg);
+  ASSERT_TRUE((bool)Database);
+  ASSERT_TRUE(ErrorMsg.empty());
+  std::vector Result = Database->getCompileCommands("source");
+  ASSERT_EQ(1ul, Result.size());
+  ASSERT_EQ(".", Result[0].Directory);
+  std::vector Expected;
+  ASSERT_THAT(Result[0].CommandLine,
+  ElementsAre(EndsWith("clang-tool"), "-target", "x86_64-linux-gnu",
+  "source"));
+  EXPECT_EQ(2, Argc);
+}
+
 struct MemCDB : public CompilationDatabase {
   using EntryMap = llvm::StringMap>;
   EntryMap Entries;
Index: clang/lib/Tooling/CompilationDatabase.cpp
===
--- clang/lib/Tooling/CompilationDatabase.cpp
+++ clang/lib/Tooling/CompilationDatabase.cpp
@@ -275,6 +275,24 @@
   Diagnostics));
   NewDriver->setCheckInputsExist(false);
 
+  // Try to infer driver mode and target from the original argv[0].
+  driver::ParsedClangName NameParts;
+  if (!Args.empty()) {
+StringRef Argv0 = Args[0];
+if (!Argv0.startswith("-") &&
+(!Argv0.startswith("/") || Argv0.count("/") > 1)) {
+  NameParts = driver::ToolChain::getTargetAndModeFromProgramName(Argv0);
+  if (NameParts.DriverMode) {
+Args.insert(Args.begin(), NameParts.DriverMode);
+  }
+
+  if (NameParts.TargetIsValid) {
+const char *arr[] = {"-target", NameParts.TargetPrefix.c_str()};
+Args.insert(Args.begin(), std::begin(arr), std::end(arr));
+  }
+}
+  }
+
   // This becomes the new argv[0]. The value is used to detect libc++ include
   // dirs on Mac, it isn't used for other platforms.
   std::string Argv0 = GetClangToolCommand();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits