[PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
mgehre updated this revision to Diff 92211.
mgehre marked an inline comment as done.
mgehre added a comment.
Update for review comments
https://reviews.llvm.org/D24886
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaStmtAttr.cpp
test/SemaCXX/suppress.cpp
Index: test/SemaCXX/suppress.cpp
===
--- /dev/null
+++ test/SemaCXX/suppress.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only %s -verify
+
+[[clang::suppress("globally")]];
+
+namespace N {
+ [[clang::suppress("in-a-namespace")]];
+}
+
+[[clang::suppress("readability-identifier-naming")]]
+void f_() {
+ int *aVar_Name;
+ [[gsl::suppress("type", "bounds")]] {
+aVar_Name = reinterpret_cast(7);
+ }
+
+ [[clang::suppress]] int x; // expected-error {{'suppress' attribute takes at least 1 argument}}
+ [[clang::suppress()]] int y; // expected-error {{'suppress' attribute takes at least 1 argument}}
+ int [[clang::suppress("r")]] z; // expected-error {{'suppress' attribute cannot be applied to types}}
+ [[clang::suppress(f_)]] float f; // expected-error {{'suppress' attribute requires a string}}
+}
+
+union [[gsl::suppress("type.1")]] U {
+ int i;
+ float f;
+};
Index: lib/Sema/SemaStmtAttr.cpp
===
--- lib/Sema/SemaStmtAttr.cpp
+++ lib/Sema/SemaStmtAttr.cpp
@@ -53,6 +53,30 @@
return ::new (S.Context) auto(Attr);
}
+static Attr *handleSuppressAttr(Sema &S, Stmt *St, const AttributeList &A,
+SourceRange Range) {
+ if (A.getNumArgs() < 1) {
+S.Diag(A.getLoc(), diag::err_attribute_too_few_arguments)
+<< A.getName() << 1;
+return nullptr;
+ }
+
+ std::vector DiagnosticIdentifiers;
+ for (unsigned I = 0, E = A.getNumArgs(); I != E; ++I) {
+StringRef RuleName;
+SourceLocation LiteralLoc;
+
+if (!S.checkStringLiteralArgumentAttr(A, I, RuleName, &LiteralLoc))
+ return nullptr;
+
+DiagnosticIdentifiers.push_back(RuleName);
+ }
+
+ return ::new (S.Context) SuppressAttr(
+ A.getRange(), S.Context, DiagnosticIdentifiers.data(),
+ DiagnosticIdentifiers.size(), A.getAttributeSpellingListIndex());
+}
+
static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const AttributeList &A,
SourceRange) {
IdentifierLoc *PragmaNameLoc = A.getArgAsIdent(0);
@@ -279,6 +303,8 @@
return handleLoopHintAttr(S, St, A, Range);
case AttributeList::AT_OpenCLUnrollHint:
return handleOpenCLUnrollHint(S, St, A, Range);
+ case AttributeList::AT_Suppress:
+return handleSuppressAttr(S, St, A, Range);
default:
// if we're here, then we parsed a known attribute, but didn't recognize
// it as a statement attribute => it is declaration attribute
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -4075,6 +4075,25 @@
}
}
+static void handleSuppressAttr(Sema &S, Decl *D, const AttributeList &Attr) {
+ if (!checkAttributeAtLeastNumArgs(S, Attr, 1))
+return;
+
+ std::vector DiagnosticIdentifiers;
+ for (unsigned I = 0, E = Attr.getNumArgs(); I != E; ++I) {
+StringRef RuleName;
+SourceLocation LiteralLoc;
+
+if (!S.checkStringLiteralArgumentAttr(Attr, I, RuleName, &LiteralLoc))
+ return;
+
+DiagnosticIdentifiers.push_back(RuleName);
+ }
+ D->addAttr(::new (S.Context) SuppressAttr(
+ Attr.getRange(), S.Context, DiagnosticIdentifiers.data(),
+ DiagnosticIdentifiers.size(), Attr.getAttributeSpellingListIndex()));
+}
+
bool Sema::CheckCallingConvAttr(const AttributeList &attr, CallingConv &CC,
const FunctionDecl *FD) {
if (attr.isInvalid())
@@ -6118,6 +6137,9 @@
case AttributeList::AT_PreserveAll:
handleCallConvAttr(S, D, Attr);
break;
+ case AttributeList::AT_Suppress:
+handleSuppressAttr(S, D, Attr);
+break;
case AttributeList::AT_OpenCLKernel:
handleSimpleAttribute(S, D, Attr);
break;
Index: include/clang/Basic/AttrDocs.td
===
--- include/clang/Basic/AttrDocs.td
+++ include/clang/Basic/AttrDocs.td
@@ -2723,6 +2723,32 @@
}];
}
+def SuppressDocs : Documentation {
+ let Category = DocCatStmt;
+ let Content = [{
+The ``[[clang::suppress]]`` and ``[[gsl::suppress]]`` attributes can be used
+to suppress specific clang-tidy warnings. The ``[[gsl::suppress]]`` is an
+alias of ``[[clang::suppress]]`` which is intended to be used for suppressing
+rules of the C++ Core Guidelines_ in a portable way. The attributes can be
+attached to declarations, statements, and on namespace scope.
+
+.. code-block: c++
+ [[clang::suppress("readability-identifier-naming")]]
+ void f_() {
+int *aVar_Name;
+[[gsl::suppress("type")]] {
+ aVar_Name = reint
[PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
aaron.ballman added a comment.
I'd also like to see some tests in Misc that confirm the attribute is being
attached to the appropriate AST nodes for declarations, statements, and at
namespace scope.
Comment at: include/clang/Basic/Attr.td:1527
+def Suppress : StmtAttr {
+ let Spellings = [CXX11<"clang", "suppress">, CXX11<"gsl", "suppress">];
+ let Args = [VariadicStringArgument<"DiagnosticIdentifiers">];
Given that this is only intended to be used with the C++ Core Guidelines, I
think we should drop the `clang::suppress` spelling for this for right now. If
we decide to expand the scope of this attribute to include more suppression
scenarios in the future, we can always bring it back.
Comment at: include/clang/Basic/AttrDocs.td:2730
+The ``[[clang::suppress]]`` and ``[[gsl::suppress]]`` attributes can be used
+to suppress specific clang-tidy warnings. The ``[[gsl::suppress]]`` is an
+alias of ``[[clang::suppress]]`` which is intended to be used for suppressing
missing the word "attribute" before "is".
Comment at: include/clang/Basic/AttrDocs.td:2733
+rules of the C++ Core Guidelines_ in a portable way. The attributes can be
+attached to declarations, statements, and on namespace scope.
+
s/on/at.
Comment at: lib/Sema/SemaDeclAttr.cpp:3851
+
+Rules.push_back(RuleName);
+ }
aaron.ballman wrote:
> Should we diagnose if asked to suppress a diagnostic that we don't support?
> e.g., someone does `[[clang::suppress("this does not exist")]]`
>
> On the one hand, we want to be forwards-compatible, but on the other hand, I
> can easily see typos in long string literals. So perhaps we may want to have
> a diagnostic based on some heuristic. e.g., we diagnose if the spelling is
> slightly off and we have viable options which we can use typo correction to
> issue a fixit, or if it's way off base, we silently eat it under the
> assumption it's a forwards compatibility thing?
I think this needs a FIXME comment. We should probably warn the user if the
rule name is unknown, but that involves a tricky layering issue since
clang-tidy checks aren't trivial to expose to the Sema layer.
Comment at: lib/Sema/SemaDeclAttr.cpp:4085
+StringRef RuleName;
+SourceLocation LiteralLoc;
+
You can get rid of `LiteralLock` and just pass `nullptr`.
Comment at: lib/Sema/SemaStmtAttr.cpp:72
+A.getAttributeSpellingListIndex());
+ return ::new (S.Context) auto(Attr);
+}
aaron.ballman wrote:
> Please construct this directly instead of relying on a copy constructor.
> Also, same comments about typos apply here as above.
Same comments about the typo FIXME applies here as well. Also, you can get rid
of `LiteralLock` and just pass `nullptr`.
https://reviews.llvm.org/D24886
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
mgehre updated this revision to Diff 92923.
mgehre added a comment.
Thank you for the comments; all of them have been addressed.
https://reviews.llvm.org/D24886
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaStmtAttr.cpp
test/SemaCXX/suppress.cpp
Index: test/SemaCXX/suppress.cpp
===
--- /dev/null
+++ test/SemaCXX/suppress.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only %s -verify
+
+[[clang::suppress("globally")]];
+
+namespace N {
+ [[clang::suppress("in-a-namespace")]];
+}
+
+[[clang::suppress("readability-identifier-naming")]]
+void f_() {
+ int *aVar_Name;
+ [[gsl::suppress("type", "bounds")]] {
+aVar_Name = reinterpret_cast(7);
+ }
+
+ [[clang::suppress]] int x; // expected-error {{'suppress' attribute takes at least 1 argument}}
+ [[clang::suppress()]] int y; // expected-error {{'suppress' attribute takes at least 1 argument}}
+ int [[clang::suppress("r")]] z; // expected-error {{'suppress' attribute cannot be applied to types}}
+ [[clang::suppress(f_)]] float f; // expected-error {{'suppress' attribute requires a string}}
+}
+
+union [[gsl::suppress("type.1")]] U {
+ int i;
+ float f;
+};
Index: lib/Sema/SemaStmtAttr.cpp
===
--- lib/Sema/SemaStmtAttr.cpp
+++ lib/Sema/SemaStmtAttr.cpp
@@ -53,6 +53,31 @@
return ::new (S.Context) auto(Attr);
}
+static Attr *handleSuppressAttr(Sema &S, Stmt *St, const AttributeList &A,
+SourceRange Range) {
+ if (A.getNumArgs() < 1) {
+S.Diag(A.getLoc(), diag::err_attribute_too_few_arguments)
+<< A.getName() << 1;
+return nullptr;
+ }
+
+ std::vector DiagnosticIdentifiers;
+ for (unsigned I = 0, E = A.getNumArgs(); I != E; ++I) {
+StringRef RuleName;
+
+if (!S.checkStringLiteralArgumentAttr(A, I, RuleName, nullptr))
+ return nullptr;
+
+// FIXME: Warn if the rule name is unknown. This is tricky because only
+// clang-tidy knows about available rules.
+DiagnosticIdentifiers.push_back(RuleName);
+ }
+
+ return ::new (S.Context) SuppressAttr(
+ A.getRange(), S.Context, DiagnosticIdentifiers.data(),
+ DiagnosticIdentifiers.size(), A.getAttributeSpellingListIndex());
+}
+
static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const AttributeList &A,
SourceRange) {
IdentifierLoc *PragmaNameLoc = A.getArgAsIdent(0);
@@ -279,6 +304,8 @@
return handleLoopHintAttr(S, St, A, Range);
case AttributeList::AT_OpenCLUnrollHint:
return handleOpenCLUnrollHint(S, St, A, Range);
+ case AttributeList::AT_Suppress:
+return handleSuppressAttr(S, St, A, Range);
default:
// if we're here, then we parsed a known attribute, but didn't recognize
// it as a statement attribute => it is declaration attribute
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -4075,6 +4075,26 @@
}
}
+static void handleSuppressAttr(Sema &S, Decl *D, const AttributeList &Attr) {
+ if (!checkAttributeAtLeastNumArgs(S, Attr, 1))
+return;
+
+ std::vector DiagnosticIdentifiers;
+ for (unsigned I = 0, E = Attr.getNumArgs(); I != E; ++I) {
+StringRef RuleName;
+
+if (!S.checkStringLiteralArgumentAttr(Attr, I, RuleName, nullptr))
+ return;
+
+// FIXME: Warn if the rule name is unknown. This is tricky because only
+// clang-tidy knows about available rules.
+DiagnosticIdentifiers.push_back(RuleName);
+ }
+ D->addAttr(::new (S.Context) SuppressAttr(
+ Attr.getRange(), S.Context, DiagnosticIdentifiers.data(),
+ DiagnosticIdentifiers.size(), Attr.getAttributeSpellingListIndex()));
+}
+
bool Sema::CheckCallingConvAttr(const AttributeList &attr, CallingConv &CC,
const FunctionDecl *FD) {
if (attr.isInvalid())
@@ -6118,6 +6138,9 @@
case AttributeList::AT_PreserveAll:
handleCallConvAttr(S, D, Attr);
break;
+ case AttributeList::AT_Suppress:
+handleSuppressAttr(S, D, Attr);
+break;
case AttributeList::AT_OpenCLKernel:
handleSimpleAttribute(S, D, Attr);
break;
Index: include/clang/Basic/AttrDocs.td
===
--- include/clang/Basic/AttrDocs.td
+++ include/clang/Basic/AttrDocs.td
@@ -2723,6 +2723,32 @@
}];
}
+def SuppressDocs : Documentation {
+ let Category = DocCatStmt;
+ let Content = [{
+The ``[[gsl::suppress]]`` attribute suppresses specific
+clang-tidy diagnostics for rules of the `C++ Core Guidelines`_ in a portable
+way. The attribute can be attached to declarations, statements, and at
+namespace scope.
+
+.. code-block:: c++
+
+ [[gsl::suppress("Rh-public")]]
+ void f_() {
+int *aVar_Name;
+[[gsl::suppress("type")]] {
+ aVar_Name =
[PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
mgehre updated this revision to Diff 93055.
mgehre added a comment.
Added test to misc; minor wording
https://reviews.llvm.org/D24886
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaStmtAttr.cpp
test/Misc/ast-dump-attr.cpp
test/SemaCXX/suppress.cpp
Index: test/SemaCXX/suppress.cpp
===
--- /dev/null
+++ test/SemaCXX/suppress.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only %s -verify
+
+[[gsl::suppress("globally")]];
+
+namespace N {
+ [[gsl::suppress("in-a-namespace")]];
+}
+
+[[gsl::suppress("readability-identifier-naming")]]
+void f_() {
+ int *p;
+ [[gsl::suppress("type", "bounds")]] {
+p = reinterpret_cast(7);
+ }
+
+ [[gsl::suppress]] int x; // expected-error {{'suppress' attribute takes at least 1 argument}}
+ [[gsl::suppress()]] int y; // expected-error {{'suppress' attribute takes at least 1 argument}}
+ int [[gsl::suppress("r")]] z; // expected-error {{'suppress' attribute cannot be applied to types}}
+ [[gsl::suppress(f_)]] float f; // expected-error {{'suppress' attribute requires a string}}
+}
+
+union [[gsl::suppress("type.1")]] U {
+ int i;
+ float f;
+};
Index: test/Misc/ast-dump-attr.cpp
===
--- test/Misc/ast-dump-attr.cpp
+++ test/Misc/ast-dump-attr.cpp
@@ -179,3 +179,25 @@
__attribute__((external_source_symbol(generated_declaration, defined_in="module", language="Swift")));
// CHECK: FunctionDecl{{.*}} TestExternalSourceSymbolAttr5
// CHECK-NEXT: ExternalSourceSymbolAttr{{.*}} "Swift" "module" GeneratedDeclaration
+
+namespace TestSuppress {
+ [[gsl::suppress("at-namespace")]];
+ // CHECK: NamespaceDecl{{.*}} TestSuppress
+ // CHECK-NEXT: EmptyDecl{{.*}}
+ // CHECK-NEXT: SuppressAttr{{.*}} at-namespace
+ [[gsl::suppress("on-decl")]]
+ void TestSuppressFunction();
+ // CHECK: FunctionDecl{{.*}} TestSuppressFunction
+ // CHECK-NEXT SuppressAttr{{.*}} on-decl
+
+ void f() {
+ int *i;
+
+ [[gsl::suppress("on-stmt")]] {
+ // CHECK: AttributedStmt
+ // CHECK-NEXT: SuppressAttr{{.*}} on-stmt
+ // CHECK-NEXT: CompoundStmt
+i = reinterpret_cast(7);
+ }
+}
+}
Index: lib/Sema/SemaStmtAttr.cpp
===
--- lib/Sema/SemaStmtAttr.cpp
+++ lib/Sema/SemaStmtAttr.cpp
@@ -53,6 +53,31 @@
return ::new (S.Context) auto(Attr);
}
+static Attr *handleSuppressAttr(Sema &S, Stmt *St, const AttributeList &A,
+SourceRange Range) {
+ if (A.getNumArgs() < 1) {
+S.Diag(A.getLoc(), diag::err_attribute_too_few_arguments)
+<< A.getName() << 1;
+return nullptr;
+ }
+
+ std::vector DiagnosticIdentifiers;
+ for (unsigned I = 0, E = A.getNumArgs(); I != E; ++I) {
+StringRef RuleName;
+
+if (!S.checkStringLiteralArgumentAttr(A, I, RuleName, nullptr))
+ return nullptr;
+
+// FIXME: Warn if the rule name is unknown. This is tricky because only
+// clang-tidy knows about available rules.
+DiagnosticIdentifiers.push_back(RuleName);
+ }
+
+ return ::new (S.Context) SuppressAttr(
+ A.getRange(), S.Context, DiagnosticIdentifiers.data(),
+ DiagnosticIdentifiers.size(), A.getAttributeSpellingListIndex());
+}
+
static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const AttributeList &A,
SourceRange) {
IdentifierLoc *PragmaNameLoc = A.getArgAsIdent(0);
@@ -279,6 +304,8 @@
return handleLoopHintAttr(S, St, A, Range);
case AttributeList::AT_OpenCLUnrollHint:
return handleOpenCLUnrollHint(S, St, A, Range);
+ case AttributeList::AT_Suppress:
+return handleSuppressAttr(S, St, A, Range);
default:
// if we're here, then we parsed a known attribute, but didn't recognize
// it as a statement attribute => it is declaration attribute
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -4075,6 +4075,26 @@
}
}
+static void handleSuppressAttr(Sema &S, Decl *D, const AttributeList &Attr) {
+ if (!checkAttributeAtLeastNumArgs(S, Attr, 1))
+return;
+
+ std::vector DiagnosticIdentifiers;
+ for (unsigned I = 0, E = Attr.getNumArgs(); I != E; ++I) {
+StringRef RuleName;
+
+if (!S.checkStringLiteralArgumentAttr(Attr, I, RuleName, nullptr))
+ return;
+
+// FIXME: Warn if the rule name is unknown. This is tricky because only
+// clang-tidy knows about available rules.
+DiagnosticIdentifiers.push_back(RuleName);
+ }
+ D->addAttr(::new (S.Context) SuppressAttr(
+ Attr.getRange(), S.Context, DiagnosticIdentifiers.data(),
+ DiagnosticIdentifiers.size(), Attr.getAttributeSpellingListIndex()));
+}
+
bool Sema::CheckCallingConvAttr(const AttributeList &attr, CallingConv &CC,
co
[PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D24886 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
This revision was automatically updated to reflect the committed changes.
Closed by commit rL298880: Add [[clang::suppress(rule, ...)]] attribute
(authored by mgehre).
Changed prior to commit:
https://reviews.llvm.org/D24886?vs=93055&id=93168#toc
Repository:
rL LLVM
https://reviews.llvm.org/D24886
Files:
cfe/trunk/include/clang/Basic/Attr.td
cfe/trunk/include/clang/Basic/AttrDocs.td
cfe/trunk/lib/Sema/SemaDeclAttr.cpp
cfe/trunk/lib/Sema/SemaStmtAttr.cpp
cfe/trunk/test/Misc/ast-dump-attr.cpp
cfe/trunk/test/SemaCXX/suppress.cpp
Index: cfe/trunk/include/clang/Basic/AttrDocs.td
===
--- cfe/trunk/include/clang/Basic/AttrDocs.td
+++ cfe/trunk/include/clang/Basic/AttrDocs.td
@@ -2771,6 +2771,32 @@
}];
}
+def SuppressDocs : Documentation {
+ let Category = DocCatStmt;
+ let Content = [{
+The ``[[gsl::suppress]]`` attribute suppresses specific
+clang-tidy diagnostics for rules of the `C++ Core Guidelines`_ in a portable
+way. The attribute can be attached to declarations, statements, and at
+namespace scope.
+
+.. code-block:: c++
+
+ [[gsl::suppress("Rh-public")]]
+ void f_() {
+int *p;
+[[gsl::suppress("type")]] {
+ p = reinterpret_cast(7);
+}
+ }
+ namespace N {
+[[clang::suppress("type", "bounds")]];
+...
+ }
+
+.. _`C++ Core Guidelines`: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#inforce-enforcement
+ }];
+}
+
def AbiTagsDocs : Documentation {
let Category = DocCatFunction;
let Content = [{
Index: cfe/trunk/include/clang/Basic/Attr.td
===
--- cfe/trunk/include/clang/Basic/Attr.td
+++ cfe/trunk/include/clang/Basic/Attr.td
@@ -1545,6 +1545,12 @@
let Documentation = [SwiftIndirectResultDocs];
}
+def Suppress : StmtAttr {
+ let Spellings = [CXX11<"gsl", "suppress">];
+ let Args = [VariadicStringArgument<"DiagnosticIdentifiers">];
+ let Documentation = [SuppressDocs];
+}
+
def SysVABI : InheritableAttr {
let Spellings = [GCC<"sysv_abi">];
// let Subjects = [Function, ObjCMethod];
Index: cfe/trunk/test/Misc/ast-dump-attr.cpp
===
--- cfe/trunk/test/Misc/ast-dump-attr.cpp
+++ cfe/trunk/test/Misc/ast-dump-attr.cpp
@@ -179,3 +179,25 @@
__attribute__((external_source_symbol(generated_declaration, defined_in="module", language="Swift")));
// CHECK: FunctionDecl{{.*}} TestExternalSourceSymbolAttr5
// CHECK-NEXT: ExternalSourceSymbolAttr{{.*}} "Swift" "module" GeneratedDeclaration
+
+namespace TestSuppress {
+ [[gsl::suppress("at-namespace")]];
+ // CHECK: NamespaceDecl{{.*}} TestSuppress
+ // CHECK-NEXT: EmptyDecl{{.*}}
+ // CHECK-NEXT: SuppressAttr{{.*}} at-namespace
+ [[gsl::suppress("on-decl")]]
+ void TestSuppressFunction();
+ // CHECK: FunctionDecl{{.*}} TestSuppressFunction
+ // CHECK-NEXT SuppressAttr{{.*}} on-decl
+
+ void f() {
+ int *i;
+
+ [[gsl::suppress("on-stmt")]] {
+ // CHECK: AttributedStmt
+ // CHECK-NEXT: SuppressAttr{{.*}} on-stmt
+ // CHECK-NEXT: CompoundStmt
+i = reinterpret_cast(7);
+ }
+}
+}
Index: cfe/trunk/test/SemaCXX/suppress.cpp
===
--- cfe/trunk/test/SemaCXX/suppress.cpp
+++ cfe/trunk/test/SemaCXX/suppress.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only %s -verify
+
+[[gsl::suppress("globally")]];
+
+namespace N {
+ [[gsl::suppress("in-a-namespace")]];
+}
+
+[[gsl::suppress("readability-identifier-naming")]]
+void f_() {
+ int *p;
+ [[gsl::suppress("type", "bounds")]] {
+p = reinterpret_cast(7);
+ }
+
+ [[gsl::suppress]] int x; // expected-error {{'suppress' attribute takes at least 1 argument}}
+ [[gsl::suppress()]] int y; // expected-error {{'suppress' attribute takes at least 1 argument}}
+ int [[gsl::suppress("r")]] z; // expected-error {{'suppress' attribute cannot be applied to types}}
+ [[gsl::suppress(f_)]] float f; // expected-error {{'suppress' attribute requires a string}}
+}
+
+union [[gsl::suppress("type.1")]] U {
+ int i;
+ float f;
+};
Index: cfe/trunk/lib/Sema/SemaStmtAttr.cpp
===
--- cfe/trunk/lib/Sema/SemaStmtAttr.cpp
+++ cfe/trunk/lib/Sema/SemaStmtAttr.cpp
@@ -53,6 +53,31 @@
return ::new (S.Context) auto(Attr);
}
+static Attr *handleSuppressAttr(Sema &S, Stmt *St, const AttributeList &A,
+SourceRange Range) {
+ if (A.getNumArgs() < 1) {
+S.Diag(A.getLoc(), diag::err_attribute_too_few_arguments)
+<< A.getName() << 1;
+return nullptr;
+ }
+
+ std::vector DiagnosticIdentifiers;
+ for (unsigned I = 0, E = A.getNumArgs(); I != E; ++I) {
+StringRef RuleName;
+
+if (!S.checkStringLiteralArgumentAttr(A, I, RuleName, nullptr))
+ return nullptr;
+
+// FIXME: Warn if the rule name is unknown. This i
[PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
roccoor added a comment.
Microsoft supports:
[[gsl::suppress(bounds.3)]]
Clang requires:
[[gsl::suppress("bounds.3")]]
Why is this inconsistency?
Here is an example from CppCoreGuidelines
[[suppress(bounds)]] char* raw_find(char* p, int n, char x)// find x in
p[0]..p[n - 1]
{
// ...
}
Repository:
rL LLVM
https://reviews.llvm.org/D24886
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
alexfh added inline comments.
Comment at: cfe/trunk/include/clang/Basic/AttrDocs.td:2792
+ namespace N {
+[[clang::suppress("type", "bounds")]];
+...
Should this be `gsl::suppress` as well?
Repository:
rL LLVM
https://reviews.llvm.org/D24886
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
mgehre added inline comments. Comment at: include/clang/Basic/AttrDocs.td:2509 +to suppress specific clang-tidy warnings. They can be attached to a scope, +statement or type. The ``[[gsl::suppress]]`` is an alias of ``[[clang::suppress]]`` +which is intended to be used for suppressing rules of the C++ Core Guidelines_ aaron.ballman wrote: > This statement does not match the test cases -- it does not appear to be > possible to attach the attribute to a type. It is true that you cannot attach the attribute to an existing type during declaration of a variable, but the attribute can be attached when declaring a new type. (See line 22 of the test case, where the attribute is attached to the union type U) I would propose to says "type declaration" instead of "type", ok? https://reviews.llvm.org/D24886 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
aaron.ballman added inline comments. Comment at: include/clang/Basic/AttrDocs.td:2509 +to suppress specific clang-tidy warnings. They can be attached to a scope, +statement or type. The ``[[gsl::suppress]]`` is an alias of ``[[clang::suppress]]`` +which is intended to be used for suppressing rules of the C++ Core Guidelines_ mgehre wrote: > aaron.ballman wrote: > > This statement does not match the test cases -- it does not appear to be > > possible to attach the attribute to a type. > It is true that you cannot attach the attribute to an existing type during > declaration of a variable, > but the attribute can be attached when declaring a new type. (See line 22 of > the test case, where the attribute is attached to the union type U) > I would propose to says "type declaration" instead of "type", ok? > I think that "type declaration" would be an improvement. https://reviews.llvm.org/D24886 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
mgehre created this revision.
mgehre added reviewers: alexfh, aaron.ballman, rsmith.
mgehre added a subscriber: cfe-commits.
This patch implements parsing of [[clang::suppress(rule, ...)]]
attributes.
C++ Core Guidelines depend heavily on tool support for
rule enforcement. They also propose a way to suppress
warnings [1] which is by annotating any ancestor in AST
with the C++11 attribute [[suppress(rule1,...)]].
I understand that clang has a policy to put all non-standard
attributes in clang namespace, thus here it is named
[[clang::suppress(rule1, ...)]].
For example, to suppress the warning cppcoreguidelines-slicing,
one could do
```
[[clang::suppress("cppcoreguidelines-slicing")]]
void f() { ... code that does slicing ... }
```
or
```
void g() {
Derived b;
[[clang::suppress("cppcoreguidelines-slicing")]]
Base a{b};
[[clang::suppress("cppcoreguidelines-slicing")]] {
doSomething();
Base a2{b};
}
}
```
This parsing can then be used by clang-tidy, which includes multiple
C++ Core Guidelines rules, to suppress warnings (patch upcoming).
For the exact naming of the rule in the attribute, there
are different possibilities, which will be defined in the
corresponding clang-tidy patch.
Currently, clang-tidy supports suppressing of warnings through "//
NOLINT" comments. There are some advantages that the attribute has:
- Supressing specific warnings instead of all warnings
- Suppressing warnings in a block (namespace, function, compound
statement)
- Code formatting may split a statement into multiple lines,
thus a "// NOLINT" comment may be on the wrong line
I could imagine to later extend this attribute further (if agreed)
- for suppressing clang-tidy warnings in general
- for suppressing clang warnings in general.
I'm looking forward to your comments!
[1]
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#inforce-enforcement
https://reviews.llvm.org/D24886
Files:
include/clang/Basic/Attr.td
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaStmtAttr.cpp
Index: lib/Sema/SemaStmtAttr.cpp
===
--- lib/Sema/SemaStmtAttr.cpp
+++ lib/Sema/SemaStmtAttr.cpp
@@ -53,6 +53,25 @@
return ::new (S.Context) auto(Attr);
}
+static Attr *handleSuppressAttr(Sema &S, Stmt *St, const AttributeList &A,
+SourceRange Range) {
+ std::vector Rules;
+
+ for (unsigned I = 0, E = A.getNumArgs(); I != E; ++I) {
+StringRef RuleName;
+SourceLocation LiteralLoc;
+
+if (!S.checkStringLiteralArgumentAttr(A, I, RuleName, &LiteralLoc))
+ return nullptr;
+
+Rules.push_back(RuleName);
+ }
+
+ SuppressAttr Attr(A.getRange(), S.Context, Rules.data(), Rules.size(),
+A.getAttributeSpellingListIndex());
+ return ::new (S.Context) auto(Attr);
+}
+
static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const AttributeList &A,
SourceRange) {
IdentifierLoc *PragmaNameLoc = A.getArgAsIdent(0);
@@ -283,6 +302,8 @@
return handleLoopHintAttr(S, St, A, Range);
case AttributeList::AT_OpenCLUnrollHint:
return handleOpenCLUnrollHint(S, St, A, Range);
+ case AttributeList::AT_Suppress:
+return handleSuppressAttr(S, St, A, Range);
default:
// if we're here, then we parsed a known attribute, but didn't recognize
// it as a statement attribute => it is declaration attribute
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -3838,6 +3838,24 @@
}
}
+static void handleSuppressAttr(Sema &S, Decl *D, const AttributeList &Attr) {
+ std::vector Rules;
+
+ for (unsigned I = 0, E = Attr.getNumArgs(); I != E; ++I) {
+StringRef RuleName;
+SourceLocation LiteralLoc;
+
+if (!S.checkStringLiteralArgumentAttr(Attr, I, RuleName, &LiteralLoc))
+ return;
+
+Rules.push_back(RuleName);
+ }
+ D->addAttr(::new (S.Context) SuppressAttr(
+ Attr.getRange(), S.Context, Rules.data(), Rules.size(),
+ Attr.getAttributeSpellingListIndex()));
+}
+
+
bool Sema::CheckCallingConvAttr(const AttributeList &attr, CallingConv &CC,
const FunctionDecl *FD) {
if (attr.isInvalid())
@@ -5768,6 +5786,9 @@
case AttributeList::AT_PreserveAll:
handleCallConvAttr(S, D, Attr);
break;
+ case AttributeList::AT_Suppress:
+handleSuppressAttr(S, D, Attr);
+break;
case AttributeList::AT_OpenCLKernel:
handleSimpleAttribute(S, D, Attr);
break;
Index: include/clang/Basic/Attr.td
===
--- include/clang/Basic/Attr.td
+++ include/clang/Basic/Attr.td
@@ -1451,6 +1451,12 @@
let Documentation = [SwiftIndirectResultDocs];
}
+def Suppress : StmtAttr {
+ let Spellings = [CXX11<"clang", "suppress">];
+ let Args = [VariadicStringArgument<"Rules">];
+ let Documentation = [Undocumented]
[PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
aaron.ballman added inline comments. Comment at: include/clang/Basic/Attr.td:1469 + let Spellings = [CXX11<"clang", "suppress">, CXX11<"gsl", "suppress">]; + let Args = [VariadicStringArgument<"Rules">]; + let Documentation = [SuppressDocs]; Perhaps we should name this something other than "Rules" -- you don't suppress a rule, you're suppressing a diagnostic based on some identifier, so I'd go with `DiagnosticIdentifiers` or something along those lines. Comment at: include/clang/Basic/AttrDocs.td:2509 +to suppress specific clang-tidy warnings. They can be attached to a scope, +statement or type. The ``[[gsl::suppress]]`` is an alias of ``[[clang::suppress]]`` +which is intended to be used for suppressing rules of the C++ Core Guidelines_ This statement does not match the test cases -- it does not appear to be possible to attach the attribute to a type. https://reviews.llvm.org/D24886 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
On Sun, Oct 29, 2017 at 4:42 AM, Hristo Hristov via Phabricator
wrote:
> roccoor added a comment.
>
> Microsoft supports:
>
> [[gsl::suppress(bounds.3)]]
>
> Clang requires:
>
> [[gsl::suppress("bounds.3")]]
>
> Why is this inconsistency?
I believe that when we added this attribute, MSVC did not support the
attribute at all and so we did what was recommended by the C++ Core
Guideline maintainers.
It would be possible for us to support the Microsoft syntax, but it
may require special parsing support.
~Aaron
>
> Here is an example from CppCoreGuidelines
>
> [[suppress(bounds)]] char* raw_find(char* p, int n, char x)// find x in
> p[0]..p[n - 1]
> {
> // ...
> }
>
>
> Repository:
> rL LLVM
>
> https://reviews.llvm.org/D24886
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
mgehre updated this revision to Diff 72366.
mgehre added a comment.
Include link to corresponding clang-tidy patch
https://reviews.llvm.org/D24886
Files:
include/clang/Basic/Attr.td
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaStmtAttr.cpp
Index: lib/Sema/SemaStmtAttr.cpp
===
--- lib/Sema/SemaStmtAttr.cpp
+++ lib/Sema/SemaStmtAttr.cpp
@@ -53,6 +53,25 @@
return ::new (S.Context) auto(Attr);
}
+static Attr *handleSuppressAttr(Sema &S, Stmt *St, const AttributeList &A,
+SourceRange Range) {
+ std::vector Rules;
+
+ for (unsigned I = 0, E = A.getNumArgs(); I != E; ++I) {
+StringRef RuleName;
+SourceLocation LiteralLoc;
+
+if (!S.checkStringLiteralArgumentAttr(A, I, RuleName, &LiteralLoc))
+ return nullptr;
+
+Rules.push_back(RuleName);
+ }
+
+ SuppressAttr Attr(A.getRange(), S.Context, Rules.data(), Rules.size(),
+A.getAttributeSpellingListIndex());
+ return ::new (S.Context) auto(Attr);
+}
+
static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const AttributeList &A,
SourceRange) {
IdentifierLoc *PragmaNameLoc = A.getArgAsIdent(0);
@@ -283,6 +302,8 @@
return handleLoopHintAttr(S, St, A, Range);
case AttributeList::AT_OpenCLUnrollHint:
return handleOpenCLUnrollHint(S, St, A, Range);
+ case AttributeList::AT_Suppress:
+return handleSuppressAttr(S, St, A, Range);
default:
// if we're here, then we parsed a known attribute, but didn't recognize
// it as a statement attribute => it is declaration attribute
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -3838,6 +3838,24 @@
}
}
+static void handleSuppressAttr(Sema &S, Decl *D, const AttributeList &Attr) {
+ std::vector Rules;
+
+ for (unsigned I = 0, E = Attr.getNumArgs(); I != E; ++I) {
+StringRef RuleName;
+SourceLocation LiteralLoc;
+
+if (!S.checkStringLiteralArgumentAttr(Attr, I, RuleName, &LiteralLoc))
+ return;
+
+Rules.push_back(RuleName);
+ }
+ D->addAttr(::new (S.Context) SuppressAttr(
+ Attr.getRange(), S.Context, Rules.data(), Rules.size(),
+ Attr.getAttributeSpellingListIndex()));
+}
+
+
bool Sema::CheckCallingConvAttr(const AttributeList &attr, CallingConv &CC,
const FunctionDecl *FD) {
if (attr.isInvalid())
@@ -5768,6 +5786,9 @@
case AttributeList::AT_PreserveAll:
handleCallConvAttr(S, D, Attr);
break;
+ case AttributeList::AT_Suppress:
+handleSuppressAttr(S, D, Attr);
+break;
case AttributeList::AT_OpenCLKernel:
handleSimpleAttribute(S, D, Attr);
break;
Index: include/clang/Basic/Attr.td
===
--- include/clang/Basic/Attr.td
+++ include/clang/Basic/Attr.td
@@ -1451,6 +1451,12 @@
let Documentation = [SwiftIndirectResultDocs];
}
+def Suppress : StmtAttr {
+ let Spellings = [CXX11<"clang", "suppress">];
+ let Args = [VariadicStringArgument<"Rules">];
+ let Documentation = [Undocumented];
+}
+
def SysVABI : InheritableAttr {
let Spellings = [GCC<"sysv_abi">];
// let Subjects = [Function, ObjCMethod];
Index: lib/Sema/SemaStmtAttr.cpp
===
--- lib/Sema/SemaStmtAttr.cpp
+++ lib/Sema/SemaStmtAttr.cpp
@@ -53,6 +53,25 @@
return ::new (S.Context) auto(Attr);
}
+static Attr *handleSuppressAttr(Sema &S, Stmt *St, const AttributeList &A,
+SourceRange Range) {
+ std::vector Rules;
+
+ for (unsigned I = 0, E = A.getNumArgs(); I != E; ++I) {
+StringRef RuleName;
+SourceLocation LiteralLoc;
+
+if (!S.checkStringLiteralArgumentAttr(A, I, RuleName, &LiteralLoc))
+ return nullptr;
+
+Rules.push_back(RuleName);
+ }
+
+ SuppressAttr Attr(A.getRange(), S.Context, Rules.data(), Rules.size(),
+A.getAttributeSpellingListIndex());
+ return ::new (S.Context) auto(Attr);
+}
+
static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const AttributeList &A,
SourceRange) {
IdentifierLoc *PragmaNameLoc = A.getArgAsIdent(0);
@@ -283,6 +302,8 @@
return handleLoopHintAttr(S, St, A, Range);
case AttributeList::AT_OpenCLUnrollHint:
return handleOpenCLUnrollHint(S, St, A, Range);
+ case AttributeList::AT_Suppress:
+return handleSuppressAttr(S, St, A, Range);
default:
// if we're here, then we parsed a known attribute, but didn't recognize
// it as a statement attribute => it is declaration attribute
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -3838,6 +3838,24 @@
}
}
+static void handleSuppressAttr(Sema &S, Decl *D, const AttributeList &Attr)
Re: [PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.
I'm generally in favor of this patch, but it's missing some pieces, like test
cases. Also, I suspect we will want this attribute to also be written on types,
but there are no changes to SemaType.cpp to handle this. Is that something you
are planning to support as well?
Comment at: include/clang/Basic/Attr.td:1457
@@ +1456,3 @@
+ let Args = [VariadicStringArgument<"Rules">];
+ let Documentation = [Undocumented];
+}
No new undocumented attributes, please.
Comment at: lib/Sema/SemaDeclAttr.cpp:3851
@@ +3850,3 @@
+
+Rules.push_back(RuleName);
+ }
Should we diagnose if asked to suppress a diagnostic that we don't support?
e.g., someone does `[[clang::suppress("this does not exist")]]`
On the one hand, we want to be forwards-compatible, but on the other hand, I
can easily see typos in long string literals. So perhaps we may want to have a
diagnostic based on some heuristic. e.g., we diagnose if the spelling is
slightly off and we have viable options which we can use typo correction to
issue a fixit, or if it's way off base, we silently eat it under the assumption
it's a forwards compatibility thing?
Comment at: lib/Sema/SemaStmtAttr.cpp:72
@@ +71,3 @@
+A.getAttributeSpellingListIndex());
+ return ::new (S.Context) auto(Attr);
+}
Please construct this directly instead of relying on a copy constructor. Also,
same comments about typos apply here as above.
https://reviews.llvm.org/D24886
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
rsmith added a comment. Giving this the name `[[clang::suppress(...)]]` seems unhelpful. This attribute is specified by the C++ core guidelines, not by clang, and presumably we want code using this attribute to be portable across implementations of the C++ code guidelines. I'd suggest asking the editors of the core guidelines what attribute namespace they'd like used, and using that here rather than using `clang`. https://reviews.llvm.org/D24886 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
aaron.ballman added a comment. In https://reviews.llvm.org/D24886#552859, @rsmith wrote: > Giving this the name `[[clang::suppress(...)]]` seems unhelpful. This > attribute is specified by the C++ core guidelines, not by clang, and > presumably we want code using this attribute to be portable across > implementations of the C++ code guidelines. I'd suggest asking the editors of > the core guidelines what attribute namespace they'd like used, and using that > here rather than using `clang`. I believe this attribute should be used to silence diagnostics for more than just the C++ Core Guidelines, so I don't think it makes sense to let them dictate what attribute namespace should be used. However, I would appreciate if they didn't suggest implementers stomp all over the attribute namespace used by standards-based attributes, either. https://reviews.llvm.org/D24886 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
mgehre added a comment. Thank your very much for your comments! Let me try to give me reasoning for those points: 1. But it's missing some pieces, like test cases I though about how to test this, having no semantic meaning itself. I could look at the AST dump, but it does not even show the rules that were passed, only that a "SuppressAttr" exists. Would that be enough? 2. Also, I suspect we will want this attribute to also be written on types I was thinking about a case were that was useful, and didn't find any. Which of course doesn't mean that there is none. I will add this. 3. No new undocumented attributes, please. I completely agree that it cannot be merged like this. This depends a bit on how our discussion turns out: Will this be specific to C++ Core Guidelines, or clang-tidy or both or none? Then, should the clang documentation mention clang-tidy? (or does that violate layering?) 4. Should we diagnose if asked to suppress a diagnostic that we don't support? I image that the users workflow would be like this: Run clang-tidy (e.g. by build server); get a warning; add [[suppress]], run clang-tidy again; see that the warning is gone. I don't see a big risk in not diagnosing a wrongly spelled suppression, because the user will notice right away that the warning is not suppressed. There is not other implicit side-effect. As an ad-don, diagnosing if the spelling is just slightly off seems like a bonus to me, but I hope that this could be deferred to another patch. 5. I'd suggest asking the editors of the core guidelines what attribute namespace they'd like used. I followed your advice and asked here: https://github.com/isocpp/CppCoreGuidelines/issues/742 I will post updates to that issue here. 6. I believe this attribute should be used to silence diagnostics for more than just the C++ Core Guidelines, so I don't think it makes sense to let them dictate what attribute namespace should be used. Maybe I wanted to much here. There are two conflicting goals: 1. Suppress C++ Core Guidelines rules in a vendor-neutral way 2. Suppress specific clang(-tidy) warnings I'm getting the feeling that we cannot have both in the same attribute. For example, the C++ Core Guidelines say that the "No reinterpret_cast" rules shall be suppressed either by saying "type" (also suppresses all other type related rules) or by saying "type.1" or by saying "type1-dont-use-reinterpret_cast". When we want to suppress other clang(-tidy) warnings, it would make sense from a usability point of view to take the warning ids that clang(-tidy) outputs. For that particular C++ Core Guideline rule, it would be "cppcoreguidelines-pro-type-reinterpret-cast". So even if we had the same attribute name for both goals, the rule names would have to differ. What are your opinions on this point? (Should I put this on the mailing list?) https://reviews.llvm.org/D24886 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
aaron.ballman added a comment. In https://reviews.llvm.org/D24886#554130, @mgehre wrote: > Thank your very much for your comments! > Let me try to give me reasoning for those points: > > 1. But it's missing some pieces, like test cases I though about how to test > this, having no semantic meaning itself. I could look at the AST dump, but it > does not even show the rules that were passed, only that a "SuppressAttr" > exists. Would that be enough? That's a good start. Other tests that are required: the attribute appertains to the proper syntactic constructs and is diagnosed otherwise, attribute has the correct number and kind of arguments, the arguments are sane, etc. > 2. Also, I suspect we will want this attribute to also be written on types I > was thinking about a case were that was useful, and didn't find any. Which of > course doesn't mean that there is none. I will add this. If there are no use cases for it, then I guess we don't need to support it. > 3. No new undocumented attributes, please. I completely agree that it cannot > be merged like this. This depends a bit on how our discussion turns out: Will > this be specific to C++ Core Guidelines, or clang-tidy or both or none? Then, > should the clang documentation mention clang-tidy? (or does that violate > layering?) I agree, we want to make sure the docs reflect our intended design. I don't think it's a problem for the clang docs to mention clang-tidy. Certainly we have LLVM documentation that mentions clang. > 4. Should we diagnose if asked to suppress a diagnostic that we don't > support? I image that the users workflow would be like this: Run clang-tidy > (e.g. by build server); get a warning; add [[suppress]], run clang-tidy > again; see that the warning is gone. I don't see a big risk in not diagnosing > a wrongly spelled suppression, because the user will notice right away that > the warning is not suppressed. There is not other implicit side-effect. I think that's definitely a reasonable use case, but I don't think it's a compelling explanation of why we should not warn the user "I have no idea what you're talking about", either. The same could be said of many diagnostics we give -- the user will notice that their code doesn't work properly. However, I tend to be in the camp of "warn on suspicious activity" camp. > As an ad-don, diagnosing if the spelling is just slightly off seems like a > bonus to me, but I hope > that this could be deferred to another patch. Certainly! > 5. I'd suggest asking the editors of the core guidelines what attribute > namespace they'd like used. I followed your advice and asked here: > https://github.com/isocpp/CppCoreGuidelines/issues/742 I will post updates to > that issue here. Thanks! > 6. I believe this attribute should be used to silence diagnostics for more > than just the C++ Core Guidelines, so I don't think it makes sense to let > them dictate what attribute namespace should be used. Maybe I wanted to much > here. There are two conflicting goals: > 7. Suppress C++ Core Guidelines rules in a vendor-neutral way > 8. Suppress specific clang(-tidy) warnings I'm getting the feeling that we > cannot have both in the same attribute. I think we do our users a major disservice by not trying to do both with the same attribute. As a user, I do not want to have to remember which way to spell an attribute to silence warnings. This is especially important were we ever to allow triggering clang-tidy diagnostics through the clang frontend. > For example, the C++ Core Guidelines say that the "No reinterpret_cast" rules > shall be suppressed either by > saying "type" (also suppresses all other type related rules) or by saying > "type.1" or by saying > "type1-dont-use-reinterpret_cast". > When we want to suppress other clang(-tidy) warnings, it would make sense > from a usability point of view > to take the warning ids that clang(-tidy) outputs. For that particular C++ > Core Guideline rule, it would be > "cppcoreguidelines-pro-type-reinterpret-cast". > So even if we had the same attribute name for both goals, the rule names > would have to differ. > > What are your opinions on this point? (Should I put this on the mailing list?) I guess I fail to see what the technical issue is (and perhaps I'm just obtuse), but I think that getting a wider audience is not a bad idea. https://reviews.llvm.org/D24886 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
malcolm.parsons added a subscriber: malcolm.parsons. malcolm.parsons added a comment. In https://reviews.llvm.org/D24886#554130, @mgehre wrote: > 2. Also, I suspect we will want this attribute to also be written on types I > was thinking about a case were that was useful, and didn't find any. Which of > course doesn't mean that there is none. I will add this. I would like to suppress cppcoreguidelines-pro-type-union-access for all member functions of a class. It might be useful to suppress cppcoreguidelines-pro-type-member-init for all constructors of a class. https://reviews.llvm.org/D24886 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
mgehre added a comment. In the C++ Core Guidelines issue on namespaceing of the [[suppress]], it was state that Microsoft uses [[gsl::suppress]] and it is the intent to update the C++ Core Guidelines to [[gsl::suppress]]. https://reviews.llvm.org/D24886 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
mgehre added a comment. My main goal is to be able to suppress any clang-tidy warning through an attribute. Initially, I though we could reuse the C++ Core Guidelines attribute for that, (because I though it was plain [[suppress]]). As a bonus (not more), we would have interoperability with other C++ Core Guideline checkers. But now that I know that it is spelled [[gsl::suppress]], I fear that this is not a good name for a general clang-tidy suppression attribute. For that, [[clang::suppress]] sounds more reasonable to me. https://reviews.llvm.org/D24886 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
