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
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]
{
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
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
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
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
te
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/
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 {
+ le
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
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 fo
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 suppr
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
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 oth
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
_
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. Wh
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 meani
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
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 a
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 th
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
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
=
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
22 matches
Mail list logo
