[PATCH] D105973: [ASTMatchers] NFC: Fix the annotation.

[liushuai wang via Phabricator via cfe-commits]

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

LGTM, thanks for your cleanup.


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

https://reviews.llvm.org/D105973

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

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

[liushuai wang via Phabricator via cfe-commits]

MTC added a comment.

In D106431#2892859 , @whisperity 
wrote:

> Is this the right decision to make, conceptually? It will leave the variable 
> uninitialised still, and reading such an uninit variable is still an issue, 
> even if it is an enum.

Yeah, that's right. However, it's much more difficult to give enum an initial 
value than an integer.

> Could we consider the alternative of warning the user about the uninitialized 
> variable, just not offering an automatic (and potentially bad / incorrect) 
> fix?

Make sense, we (ByteDance) are also hesitating whether we should provide 
automatic repair for uninitialized variables, because automatic fix may change 
the program semantics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106431

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

[PATCH] D106431: [clang-tidy] Fix cppcoreguidelines-init-variables with enum judgement

[liushuai wang via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-init-variables.cpp:103
+  // Expect no warning given here.
+  Color color;
+  // Expect no warning given here.

steven.zhang wrote:
> Technical speaking, we should warn about such case, ad the color is undefined 
> now. Can we initialize it to the lowerest value of the enum type ?
Good point! However, which value should be chosen as automatic fix is a problem 
worth discussing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106431

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

[PATCH] D123924: [clang-apply-replacements] Added an option to ignore insert conflict.

[liushuai wang via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: clang/include/clang/Tooling/Refactoring/AtomicChange.h:119
 
+  Replacements &getRefReplacements() { return Replaces; }
+

IMHO, there is no need to rename the method to get the non-const version.  The 
following code is ok enough

`Replacements &getReplacements() { return Replaces; }`


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

https://reviews.llvm.org/D123924

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

[PATCH] D123924: [clang-apply-replacements] Added an option to ignore insert conflict.

[liushuai wang via Phabricator via cfe-commits]

MTC added a comment.

In D123924#3482342 , @aaron.ballman 
wrote:

> I don't know enough about this code base to feel comfortable signing off on 
> it, but the changes look reasonable to me FWIW.

Thanks, Aaron! I will find another guy to review the change.


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

https://reviews.llvm.org/D123924

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

[PATCH] D107450: [clang-tidy] Fix wrong and missing warnings in performance-move-const-arg

[liushuai wang via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:75
+  return std::move(x4); 
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: passing result of std::move() 
as a const reference argument; no move will actually happen
+  // CHECK-FIXES: return x4;

Quuxplusone wrote:
> You're right that //some// diagnostic should be given here, but //this// is 
> not the right diagnostic. The issue is not that the `std::move` is 
> non-functional, but rather that it's superfluous; a move //will// happen, 
> with or without the `std::move`. (And the `std::move` prevents move elision, 
> so removing it may actually improve codegen.)
Correct!  @Sockke would you self-examination these cases again? I have checked 
this case, see https://godbolt.org/z/3ch3sbG17.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:261
+  showInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 'a' 
of the trivially-copyable type 'int' has no effect; remove std::move()
+  return std::move(a);

Quuxplusone wrote:
> This warning is incorrect, as indicated in your PR summary. Isn't the point 
> of this PR mainly to remove this incorrect warning?
I guess what @Sockke wants to express is that applying `std::move` on the 
integer type makes no sense.


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

https://reviews.llvm.org/D107450

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

[PATCH] D107641: [clang-tidy] fix duplicate '{}' in cppcoreguidelines-pro-type-member-init

[liushuai wang via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:36
 template 
 void forEachField(const RecordDecl &Record, const T &Fields, Func &&Fn) {
   for (const FieldDecl *F : Fields) {

Btw, the parameter `Record` seems useless, could you clean it up in another 
patch?



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:439
+   [&](const FieldDecl *F) { 
+if (!HasRecordClasMemberSet.count(F))
+{

I believe `DenseSet::contains` is more appropriate here.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:454
+
+  if (AllFieldsToInit.empty())
+return;

`FieldsToInit` being empty implies `AllFieldsToInit` is empty. We have checked 
the emptiness of `FieldsToInit`.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp:211
 
   // FIXME: The fix-its here collide providing an erroneous fix
   int A, B;

Please update the comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107641

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

[PATCH] D107641: [clang-tidy] fix duplicate '{}' in cppcoreguidelines-pro-type-member-init

[liushuai wang via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:454
+
+  if (AllFieldsToInit.empty())
+return;

MTC wrote:
> `FieldsToInit` being empty implies `AllFieldsToInit` is empty. We have 
> checked the emptiness of `FieldsToInit`.
never mind, what I said is wrong！


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107641

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

[PATCH] D107450: [clang-tidy] Fix wrong and missing warnings in performance-move-const-arg

[liushuai wang via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:261
+  showInt(std::move(a));
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 'a' 
of the trivially-copyable type 'int' has no effect; consider changing showInt's 
parameter from 'int'&& to 'int'&
+  return std::move(a);

Change **'int'&&**  -> **'int&&'** and **'int&'** -> **int**. 

Make `consider changing showInt's parameter from 'int'&& to 'int'&` as a note 
instead of a warning. And I don't have a strong preference for the position of 
the note, but I personally want to put it in the source location of the 
function definition. and 


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

https://reviews.llvm.org/D107450

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

[PATCH] D107450: [clang-tidy] Fix wrong and missing warnings in performance-move-const-arg

[liushuai wang via Phabricator via cfe-commits]

MTC added a comment.

I think we're a bit off track,  and @Sockke wants to accomplish more than one 
goal in the same patch. I have summarized what we are currently discussing as 
follow shows:

1. Fix the wrong AutoFix which blocks the compilation.
2. Find more 'unrecommended' std::move usage and give correct warning messages.
3. Whether template should be taken into account.

In addition, I would like to mention that we need to ensure that this check 
should be consistent with `-Wpessimizing-move`, see 
https://reviews.llvm.org/D7633, which has done the perfect job.

I suggest that this patch be divided into two patches. In the current patch, 
fix the **wrong AutoFix**. What the current check should look like is left in 
the second patch for discussion. @Sockke do you mind simplifying this patch and 
only achieving the first goal?




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp:284
+  Tmp t2 = std::move(Tmp());
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: std::move of the expression of 
the trivially-copyable type 'Tmp' has no effect; remove std::move()
+  // CHECK-FIXES: Tmp t2 = Tmp();

Quuxplusone wrote:
> Here, the more immediate red flag is that the programmer is using `std::move` 
> on an rvalue expression.
> ```
> std::string&& a();
> std::string& b();
> std::string c();
> auto aa = std::move(a());  // not great, moving an xvalue
> auto bb = std::move(b());  // OK, moving an lvalue
> auto cc = std::move(c());  // not great, moving a prvalue
> ```
> However, the more complicated and guess-worky these diagnostics get, the more 
> I want to see how they interact with templates and generic code.
> ```
> template
> auto w(F f) {
> return std::move(f());
> }
> std::string callw1() { std::string s; return w([&]() -> std::string { return 
> s; });
> std::string callw2() { std::string s; return w([&]() -> std::string& { return 
> s; });
> ```
> `callw1` ends up applying `std::move` to a prvalue inside `w`, but when 
> `callw2` calls `w`, the `std::move` actually does something. Possible 
> strategies here include "Warn on code that's only sometimes correct," "Don't 
> warn on code that's only sometimes wrong," and "Don't implement the 
> diagnostic at all because it's too icky."
Agree! For `Tmp t2 = std::move(Tmp());`, what should be reported here is moving 
a **prvalue**. However, the original behavior is emitting this message.

And the original matcher will ignore template instantiation, see 
`unless(isInTemplateInstantiation())`. 


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

https://reviews.llvm.org/D107450

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

[PATCH] D107641: [clang-tidy] fix duplicate '{}' in cppcoreguidelines-pro-type-member-init

[liushuai wang via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1f2d40c47f5f: [clang-tidy] fix duplicate '{}' in 
cppcoreguidelines-pro-type-member-init (authored by Sockke, committed by MTC).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107641

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
@@ -208,9 +208,8 @@
   PositiveMultipleConstructors(const PositiveMultipleConstructors &) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize 
these fields: A, B
 
-  // FIXME: The fix-its here collide providing an erroneous fix
   int A, B;
-  // CHECK-FIXES: int A{}{}{}, B{}{}{};
+  // CHECK-FIXES: int A{}, B{};
 };
 
 typedef struct {
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h
@@ -10,6 +10,7 @@
 #define 
LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_TYPE_MEMBER_INIT_H
 
 #include "../ClangTidyCheck.h"
+#include "llvm/ADT/DenseSet.h"
 
 namespace clang {
 namespace tidy {
@@ -72,6 +73,10 @@
   // instead of brace initialization. Only effective in C++11 mode. Default is
   // false.
   bool UseAssignment;
+
+  // Record the member variables that have been initialized to prevent repeated
+  // initialization.
+  llvm::DenseSet HasRecordClassMemberSet;
 };
 
 } // namespace cppcoreguidelines
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
@@ -433,17 +433,25 @@
[&](const FieldDecl *F) { OrderedFields.push_back(F); });
 
   // Collect all the fields we need to initialize, including indirect fields.
+  // It only includes fields that have not been fixed
   SmallPtrSet AllFieldsToInit;
-  forEachField(ClassDecl, FieldsToInit,
-   [&](const FieldDecl *F) { AllFieldsToInit.insert(F); });
-  if (AllFieldsToInit.empty())
+  forEachField(ClassDecl, FieldsToInit, [&](const FieldDecl *F) {
+if (!HasRecordClassMemberSet.contains(F)) {
+  AllFieldsToInit.insert(F);
+  HasRecordClassMemberSet.insert(F);
+}
+  });
+  if (FieldsToInit.empty())
 return;
 
   DiagnosticBuilder Diag =
   diag(Ctor ? Ctor->getBeginLoc() : ClassDecl.getLocation(),
"%select{|union }0constructor %select{does not|should}0 initialize "
"%select{|one of }0these fields: %1")
-  << IsUnion << toCommaSeparatedString(OrderedFields, AllFieldsToInit);
+  << IsUnion << toCommaSeparatedString(OrderedFields, FieldsToInit);
+
+  if (AllFieldsToInit.empty())
+return;
 
   // Do not propose fixes for constructors in macros since we cannot place them
   // correctly.


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
@@ -208,9 +208,8 @@
   PositiveMultipleConstructors(const PositiveMultipleConstructors &) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: A, B
 
-  // FIXME: The fix-its here collide providing an erroneous fix
   int A, B;
-  // CHECK-FIXES: int A{}{}{}, B{}{}{};
+  // CHECK-FIXES: int A{}, B{};
 };
 
 typedef struct {
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h
@@ -10,6 +10,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_TYPE_MEMBER_INIT_H
 
 #include "../ClangTidyCheck.h"
+#include "llvm/ADT/DenseSet.h"
 
 namespace clang {
 namespace tidy {
@@ -72,6 +73,10 @@
   // instead of brace initialization. Only effective in C++11 mode. Default is
   // false.
   bool UseAssignment;
+
+

[PATCH] D67149: Fix argument order for BugType instation for

[liushuai wang via Phabricator via cfe-commits]

MTC added a comment.
Herald added a subscriber: martong.

Hi @NoQ, sorry to bother you. This patch has been on hold for a very long time. 
It should not have been fixed yet. What's next? Abandon the patch, or improve 
it yourself?


Repository:
  rC Clang

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

https://reviews.llvm.org/D67149

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

[PATCH] D108370: [clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[liushuai wang via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:49
+void forEachFieldWithFilter(const RecordDecl &Record, const T &Fields,
+bool &AnyMemberHasInitPerUnion, Func &&Fn) {
+  for (const FieldDecl *F : Fields) {

Can it be modified to the following form? Or further abstract `filter` into a 
parameter to make this function more general.


```
template 
void forEachFieldWithFilter(const RecordDecl &Record, const T &Fields,
bool &AnyMemberHasInitPerUnion, Func &&Fn) {
  forEachField(Record, Fields, Fn);
  if (Record.isUnion() && AnyMemberHasInitPerUnion)
break;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108370

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

[PATCH] D133563: [Clang] Improve diagnostics about the invalid target feature.

[liushuai wang via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG910ad36e1a25: [Clang] Improve diagnostics about the invalid 
target feature. (authored by MTC).
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133563

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/lib/Basic/TargetInfo.cpp
  clang/test/Frontend/invalid-target-feature.c


Index: clang/test/Frontend/invalid-target-feature.c
===
--- /dev/null
+++ clang/test/Frontend/invalid-target-feature.c
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -target-feature '' 
-target-feature m %s 2>&1 \
+// RUN:| FileCheck -check-prefix=NO_PREFIX %s
+
+// NO_PREFIX: warning: feature flag 'm' must start with either '+' to enable 
the feature or '-' to disable it; flag ignored [-Winvalid-command-line-argument]
+
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -target-feature '' 
-target-feature ' n' %s 2>&1 \
+// RUN:| FileCheck -check-prefix=NULL_PREFIX %s
+
+// NULL_PREFIX: warning: feature flag ' n' must start with either '+' to 
enable the feature or '-' to disable it; flag ignored 
[-Winvalid-command-line-argument]
Index: clang/lib/Basic/TargetInfo.cpp
===
--- clang/lib/Basic/TargetInfo.cpp
+++ clang/lib/Basic/TargetInfo.cpp
@@ -14,6 +14,7 @@
 #include "clang/Basic/AddressSpaces.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticFrontend.h"
 #include "clang/Basic/LangOptions.h"
 #include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/STLExtras.h"
@@ -494,9 +495,13 @@
 const std::vector &FeatureVec) const {
   for (const auto &F : FeatureVec) {
 StringRef Name = F;
+if (Name.empty())
+  continue;
 // Apply the feature via the target.
-bool Enabled = Name[0] == '+';
-setFeatureEnabled(Features, Name.substr(1), Enabled);
+if (Name[0] != '+' && Name[0] != '-')
+  Diags.Report(diag::warn_fe_backend_invalid_feature_flag) << Name;
+else
+  setFeatureEnabled(Features, Name.substr(1), Name[0] == '+');
   }
   return true;
 }
Index: clang/include/clang/Basic/DiagnosticFrontendKinds.td
===
--- clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ clang/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -47,6 +47,9 @@
 def warn_fe_backend_unsupported_fp_exceptions : Warning<
 "overriding currently unsupported use of floating point exceptions "
 "on this target">, InGroup;
+def warn_fe_backend_invalid_feature_flag : Warning<
+"feature flag '%0' must start with either '+' to enable the feature or '-'"
+" to disable it; flag ignored">, InGroup;
 
 def err_incompatible_fp_eval_method_options : Error<
 "option 'ffp-eval-method' cannot be used with option "


Index: clang/test/Frontend/invalid-target-feature.c
===
--- /dev/null
+++ clang/test/Frontend/invalid-target-feature.c
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -target-feature '' -target-feature m %s 2>&1 \
+// RUN:| FileCheck -check-prefix=NO_PREFIX %s
+
+// NO_PREFIX: warning: feature flag 'm' must start with either '+' to enable the feature or '-' to disable it; flag ignored [-Winvalid-command-line-argument]
+
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -target-feature '' -target-feature ' n' %s 2>&1 \
+// RUN:| FileCheck -check-prefix=NULL_PREFIX %s
+
+// NULL_PREFIX: warning: feature flag ' n' must start with either '+' to enable the feature or '-' to disable it; flag ignored [-Winvalid-command-line-argument]
Index: clang/lib/Basic/TargetInfo.cpp
===
--- clang/lib/Basic/TargetInfo.cpp
+++ clang/lib/Basic/TargetInfo.cpp
@@ -14,6 +14,7 @@
 #include "clang/Basic/AddressSpaces.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticFrontend.h"
 #include "clang/Basic/LangOptions.h"
 #include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/STLExtras.h"
@@ -494,9 +495,13 @@
 const std::vector &FeatureVec) const {
   for (const auto &F : FeatureVec) {
 StringRef Name = F;
+if (Name.empty())
+  continue;
 // Apply the feature via the target.
-bool Enabled = Name[0] == '+';
-setFeatureEnabled(Features, Name.substr(1), Enabled);
+if (Name[0] != '+' && Name[0] != '-')
+  Diags.Report(diag::warn_fe_backend_invalid_feature_flag) << Name;
+else
+  setFeatureEnabled(Features, Name.substr(1), Name[0] == '+');
   }
   return true;
 }
Index: clang/include/clang/Basic/DiagnosticFrontendKinds.td
===
--- clang/include/clang/Basic/Diagnost

[PATCH] D128207: [clang-doc][NFC] Fix reference invalidation assertion failure.

[liushuai wang via Phabricator via cfe-commits]

MTC created this revision.
MTC added reviewers: dexonsmith, mehdi_amini.
MTC added a project: clang-tools-extra.
Herald added subscribers: jsji, usaxena95, pengfei, kadircet, arphaman.
Herald added a project: All.
MTC requested review of this revision.
Herald added subscribers: cfe-commits, ilya-biryukov.

Fix the assertion failure caused by reference invalidation in `SmallString`. It 
seems that this bug was not caught when adding the assertion, see 
https://reviews.llvm.org/D91744.

We can reproduce this bug trivially through the following usage.

  $ clang-doc  --format=html test.cpp
  Emiting docs in html format.
  clang-doc: llvm-project/llvm/include/llvm/ADT/SmallVector.h:173: void 
llvm::SmallVectorTemplateCommon 
>::assertSafeToReferenceAfterResize(const void*, size_t) [with T = char; 
 = void; size_t = long unsigned int]: Assertion 
`isSafeToReferenceAfterResize(Elt, NewSize) && "Attempting to reference an 
element of the vector in an operation " "that invalidates it"' failed.
   #0 0x55ce4806b843 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) 
llvm-project/llvm/lib/Support/Unix/Signals.inc:569:22
   #1 0x55ce4806b8fa PrintStackTraceSignalHandler(void*) 
llvm-project/llvm/lib/Support/Unix/Signals.inc:636:1
   #2 0x55ce4806989a llvm::sys::RunSignalHandlers() 
llvm-project/llvm/lib/Support/Signals.cpp:103:20
   #3 0x55ce4806b277 SignalHandler(int) 
llvm-project/llvm/lib/Support/Unix/Signals.inc:407:1
   #4 0x7f72a4533730 __restore_rt 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x12730)
   #5 0x7f72a3e1a7bb raise 
/build/glibc-fWwxX8/glibc-2.28/signal/../sysdeps/unix/sysv/linux/raise.c:51:1
   #6 0x7f72a3e05535 abort 
/build/glibc-fWwxX8/glibc-2.28/stdlib/abort.c:81:7
   #7 0x7f72a3e0540f _nl_load_domain 
/build/glibc-fWwxX8/glibc-2.28/intl/loadmsgcat.c:1177:9
   #8 0x7f72a3e13102 (/lib/x86_64-linux-gnu/libc.so.6+0x30102)
   #9 0x55ce47f76f1a llvm::SmallVectorTemplateCommon::assertSafeToReferenceAfterResize(void const*, unsigned long) 
llvm-project/llvm/include/llvm/ADT/SmallVector.h:171:5
  #10 0x55ce47f79efc llvm::SmallVectorTemplateCommon::assertSafeToReferenceAfterClear(char const*, char const*) 
llvm-project/llvm/include/llvm/ADT/SmallVector.h:187:47
  #11 0x55ce47f743ed void llvm::SmallVectorImpl::assign(char const*, char const*) 
llvm-project/llvm/include/llvm/ADT/SmallVector.h:713:5
  #12 0x55ce47f6c587 llvm::SmallString<128u>::assign(llvm::StringRef) 
llvm-project/llvm/include/llvm/ADT/SmallString.h:53:3
  #13 0x55ce47f64338 llvm::SmallString<128u>::operator=(llvm::StringRef) 
llvm-project/llvm/include/llvm/ADT/SmallString.h:279:13
  #14 0x55ce47f4ab4c main 
llvm-project/clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:226:28
  #15 0x7f72a3e0709b __libc_start_main 
/build/glibc-fWwxX8/glibc-2.28/csu/../csu/libc-start.c:342:3
  #16 0x55ce47f4963a _start (./bin/clang-doc+0x54563a)
  [1]1443196 abort (core dumped)  ./bin/clang-doc --format=html test.cpp


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D128207

Files:
  clang-tools-extra/clang-doc/tool/ClangDocMain.cpp


Index: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp
===
--- clang-tools-extra/clang-doc/tool/ClangDocMain.cpp
+++ clang-tools-extra/clang-doc/tool/ClangDocMain.cpp
@@ -222,7 +222,7 @@
 std::string ClangDocPath = GetExecutablePath(argv[0], MainAddr);
 llvm::SmallString<128> AssetsPath;
 llvm::sys::path::native(ClangDocPath, AssetsPath);
-AssetsPath = llvm::sys::path::parent_path(AssetsPath);
+AssetsPath = std::string(llvm::sys::path::parent_path(AssetsPath));
 llvm::sys::path::append(AssetsPath, "..", "share", "clang");
 llvm::SmallString<128> DefaultStylesheet;
 llvm::sys::path::native(AssetsPath, DefaultStylesheet);


Index: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp
===
--- clang-tools-extra/clang-doc/tool/ClangDocMain.cpp
+++ clang-tools-extra/clang-doc/tool/ClangDocMain.cpp
@@ -222,7 +222,7 @@
 std::string ClangDocPath = GetExecutablePath(argv[0], MainAddr);
 llvm::SmallString<128> AssetsPath;
 llvm::sys::path::native(ClangDocPath, AssetsPath);
-AssetsPath = llvm::sys::path::parent_path(AssetsPath);
+AssetsPath = std::string(llvm::sys::path::parent_path(AssetsPath));
 llvm::sys::path::append(AssetsPath, "..", "share", "clang");
 llvm::SmallString<128> DefaultStylesheet;
 llvm::sys::path::native(AssetsPath, DefaultStylesheet);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128207: [clang-doc][NFC] Fix reference invalidation assertion failure.

[liushuai wang via Phabricator via cfe-commits]

MTC added a comment.

Sorry for the delayed response. Adding the regression test is not as simple as 
I thought, **first of all clang-tools-extra does not enable the //assert//**, 
see 
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/lit.site.cfg.py.in
 . And add the clang-doc when enable `--format=html` is little bit of tricky.

> Also, I wonder if assign could/should add logic to be resilient to this.

Eventhough `destroy_range()` does nothing for now, there are indeed potential 
risks.

  void assign(in_iter in_start, in_iter in_end) {
this->assertSafeToReferenceAfterClear(in_start, in_end);
clear();
append(in_start, in_end);
  }
  void clear() {
this->destroy_range(this->begin(), this->end());
this->Size = 0;
  }


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D128207

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

[PATCH] D117090: [clang-tidy] Fix `readability-non-const-parameter` for parameter referenced by an lvalue

[liushuai wang via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp:293
+  // CHECK-MESSAGES-NOT: warning: pointer parameter 'p' can be
+  int &x = *p;
+}

@Sockke  Could you please add the following tests?

```
int &ref = std::ref(*p);
const int &cref = std::ref(*p);
const int &cref = std::cref(*p);
const int *ptr = std::as_const(p);
int *ptr = const_cast(std::as_const(p));
decltype(auto) ptr = p;
auto ptr = p;
```


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

https://reviews.llvm.org/D117090

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

[PATCH] D117090: [clang-tidy] Fix `readability-non-const-parameter` for parameter referenced by an lvalue

[liushuai wang via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp:293
+  // CHECK-MESSAGES-NOT: warning: pointer parameter 'p' can be
+  int &x = *p;
+}

Sockke wrote:
> MTC wrote:
> > @Sockke  Could you please add the following tests?
> > 
> > ```
> > int &ref = std::ref(*p);
> > const int &cref = std::ref(*p);
> > const int &cref = std::cref(*p);
> > const int *ptr = std::as_const(p);
> > int *ptr = const_cast(std::as_const(p));
> > decltype(auto) ptr = p;
> > auto ptr = p;
> > ```
> These cases have been handled stably in another logic.
Sorry for my mistake, look good to me!


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

https://reviews.llvm.org/D117090

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

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[liushuai wang via Phabricator via cfe-commits]

MTC added a comment.

This patch has become very complicated now. I summarized this patch and give a 
figure to illustrate what we have reached. And @Sockke please add some comments 
to explain the complex part or other means to make this patch more readable.

F21496057: D107450.svg 


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

https://reviews.llvm.org/D107450

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

[PATCH] D116439: [clang-tidy] Fix `readability-const-return-type` for pure virtual function.

[liushuai wang via Phabricator via cfe-commits]

MTC added subscribers: flx, MTC.
MTC added a comment.

FYI: I'm from Bytedance Inc , @Sockke, and I are 
fixing the AutoFix bugs recently, most of them will lead to the compilation 
error.  For this bug, fixing the override virtual methods but missing the pure 
virtual base method, which will cause the compilation error. These annoying 
bugs will hinder the large-scale deployment of clang-tidy AutoFix in the 
production environment.




Comment at: 
clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp:101
+  functionDecl(returns(isConstQualified()),
+   anyOf(isDefinition(), cxxMethodDecl(isPure(
+  .bind("func"),

Like @flx comments in https://reviews.llvm.org/D116593, the better choice is 
that we suppress the fix for the virtual method. What do you think @Sockke?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116439

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

[PATCH] D117090: Fix `readability-non-const-parameter` for parameter referenced by an lvalue

[liushuai wang via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp:86-99
+  // Data passed by nonconst reference should not be made const.
+  unsigned ArgNr = 0U;
+  if (const auto *CD = CE->getConstructor()) {
+for (const auto *Par : CD->parameters()) {
+  if (ArgNr >= CE->getNumArgs())
+break;
+  const Expr *Arg = CE->getArg(ArgNr++);

`86~99` is pretty close to `64~81`, could you please refactor it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117090

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