[PATCH] D56563: [clang-tidy] add options documentation to readability-identifier-naming checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 181224.
MyDeveloperDay added a comment.

Address review comments

- clang-format the code examples
- replace "ensure" with "check"


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

https://reviews.llvm.org/D56563

Files:
  docs/clang-tidy/checks/readability-identifier-naming.rst

Index: docs/clang-tidy/checks/readability-identifier-naming.rst
===
--- docs/clang-tidy/checks/readability-identifier-naming.rst
+++ docs/clang-tidy/checks/readability-identifier-naming.rst
@@ -5,14 +5,1867 @@
 
 Checks for identifiers naming style mismatch.
 
-This check will try to enforce coding guidelines on the identifiers naming.
-It supports `lower_case`, `UPPER_CASE`, `camelBack` and `CamelCase` casing and
-tries to convert from one to another if a mismatch is detected.
+This check will try to enforce coding guidelines on the identifiers naming. It
+supports one the following casing types and tries to convert from one to
+another if a mismatch is detected
 
-It also supports a fixed prefix and suffix that will be prepended or
-appended to the identifiers, regardless of the casing.
+Casing types inclde:
+
+ - ``lower_case``,
+ - ``UPPER_CASE``,
+ - ``camelBack``,
+ - ``CamelCase``,
+ - ``camel_Snake_Back``,
+ - ``Camel_Snake_Case``,
+ - ``aNy_CasE``.
+
+It also supports a fixed prefix and suffix that will be prepended or appended
+to the identifiers, regardless of the casing.
 
 Many configuration options are available, in order to be able to create
-different rules for different kind of identifier. In general, the
-rules are falling back to a more generic rule if the specific case is not
-configured.
+different rules for different kinds of identifier. In general, the rules are
+falling back to a more generic rule if the specific case is not configured.
+
+Options
+---
+
+.. option:: AbstractClassCase
+
+When defined, the checker will check abstract class names conform to the
+selected casing.
+
+.. option:: AbstractClassPrefix
+
+When defined, the checker will check abstract class names will add the
+prefixed with the given value (regardless of casing).
+
+.. option:: AbstractClassSuffix
+
+When defined, the checker will check abstract class names will add the
+suffix with the given value (regardless of casing).
+
+For example using values of:
+
+   - AbstractClassCase of ``lower_case``
+   - AbstractClassPrefix of ``pre_``
+   - AbstractClassSuffix of ``_post``
+
+Identifies and/or transforms abstract class names as follows:
+
+Before:
+
+.. code-block:: c++
+
+class ABSTRACT_CLASS {
+public:
+  ABSTRACT_CLASS();
+};
+
+After:
+
+.. code-block:: c++
+
+class abstract_class {
+public:
+  pre_abstract_class_post();
+};
+
+.. option:: ClassCase
+
+When defined, the checker will check class names conform to the
+selected casing.
+
+.. option:: ClassPrefix
+
+When defined, the checker will check class names will add the
+prefixed with the given value (regardless of casing).
+
+.. option:: ClassSuffix
+
+When defined, the checker will check class names will add the
+suffix with the given value (regardless of casing).
+
+For example using values of:
+
+   - ClassCase of ``lower_case``
+   - ClassPrefix of ``pre_``
+   - ClassSuffix of ``_post``
+
+Identifies and/or transforms class names as follows:
+
+Before:
+
+.. code-block:: c++
+
+class FOO {
+public:
+  FOO();
+  ~FOO();
+};
+
+After:
+
+.. code-block:: c++
+
+class pre_foo_post {
+public:
+  pre_foo_post();
+  ~pre_foo_post();
+};
+
+.. option:: ClassConstantCase
+
+When defined, the checker will check class constant names conform to the
+selected casing.
+
+.. option:: ClassConstantPrefix
+
+When defined, the checker will check class constant names will add the
+prefixed with the given value (regardless of casing).
+
+.. option:: ClassConstantSuffix
+
+When defined, the checker will check class constant names will add the
+suffix with the given value (regardless of casing).
+
+For example using values of:
+
+   - ClassConstantCase of ``lower_case``
+   - ClassConstantPrefix of ``pre_``
+   - ClassConstantSuffix of ``_post``
+
+Identifies and/or transforms class constant names as follows:
+
+Before:
+
+.. code-block:: c++
+
+class FOO {
+public:
+  static const int CLASS_CONSTANT;
+};
+
+After:
+
+.. code-block:: c++
+
+class FOO {
+public:
+  static const int pre_class_constant_post;
+};
+
+.. option:: ClassMemberCase
+
+When defined, the checker will check class member names conform to the
+selected casing.
+
+.. option:: ClassMemberPrefix
+
+When defined, the checker will check class member names will add the
+prefixed with the given value (regardless of casing).
+
+.. option:: ClassMemberSuffix
+
+When defined, the checker will check class member 

[PATCH] D56563: [clang-tidy] add options documentation to readability-identifier-naming checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 5 inline comments as done.
MyDeveloperDay added a comment.

> I think will be good idea to create generic Case/Prefix/Suffix description to 
> reduce size of documentation.

I'm generating this documentation via a script, using the following template, 
(which is why its quite repetitive), the grunt work was in making the examples 
which are stored in separate Option_Before.cpp/Option_After.cpp file.

This was actually my second pass, initially I called out each option 
individually but that made for 100's of example files, if you think this text 
could be generalized even more I'd be happy to regenerate it. (I'm not much of 
a wordsmith)

> .. option:: %name%Case
> 
>   When defined, the checker will check %desc% names conform to the
>   selected casing.
>
> 
> .. option:: %name%Prefix
> 
>   When defined, the checker will check %desc% names will add the
>   prefixed with the given value (regardless of casing).
>
> 
> .. option:: %name%Suffix
> 
>   When defined, the checker will check %desc% names will add the
>   suffix with the given value (regardless of casing).
>
> 
> For example using values of:
> 
> - %name%Case of ``lower_case``
> - %name%Prefix of ``pre_``
> - %name%Suffix of ``_post``
> 
>   Identifies and/or transforms %desc% names as follows:
> 
>   Before:
> 
>   .. code-block:: c++ %before_example%
> 
>   After:
> 
>   .. code-block:: c++ %after_example%






Comment at: docs/clang-tidy/checks/readability-identifier-naming.rst:34
+
+When defined, the checker will ensure abstract class names conform to the
+selected casing.

Eugene.Zelenko wrote:
> check, please. Same in other places.
I assumed you mean replace "ensure" with "check", now I'm not sure do you mean?

A) "the checker will check"
B) "the check will ensure"
C) "the check will check"




Comment at: docs/clang-tidy/checks/readability-identifier-naming.rst:278
+{
+unsigned const MyConst_array[] = {1,2,3};
+}

Eugene.Zelenko wrote:
> Will be good idea to run Clang-format over code snippets.
so I formatted the examples but public didn't get indented, let me know if you 
think I've got the format wrong


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

https://reviews.llvm.org/D56563



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

[PATCH] D56563: [clang-tidy] add options documentation to readability-identifier-naming checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 181497.
MyDeveloperDay added a comment.

Fix review comments

s/the checker will check/the check will ensure/


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

https://reviews.llvm.org/D56563

Files:
  docs/clang-tidy/checks/readability-identifier-naming.rst

Index: docs/clang-tidy/checks/readability-identifier-naming.rst
===
--- docs/clang-tidy/checks/readability-identifier-naming.rst
+++ docs/clang-tidy/checks/readability-identifier-naming.rst
@@ -5,14 +5,1867 @@
 
 Checks for identifiers naming style mismatch.
 
-This check will try to enforce coding guidelines on the identifiers naming.
-It supports `lower_case`, `UPPER_CASE`, `camelBack` and `CamelCase` casing and
-tries to convert from one to another if a mismatch is detected.
+This check will try to enforce coding guidelines on the identifiers naming. It
+supports one the following casing types and tries to convert from one to
+another if a mismatch is detected
 
-It also supports a fixed prefix and suffix that will be prepended or
-appended to the identifiers, regardless of the casing.
+Casing types inclde:
+
+ - ``lower_case``,
+ - ``UPPER_CASE``,
+ - ``camelBack``,
+ - ``CamelCase``,
+ - ``camel_Snake_Back``,
+ - ``Camel_Snake_Case``,
+ - ``aNy_CasE``.
+
+It also supports a fixed prefix and suffix that will be prepended or appended
+to the identifiers, regardless of the casing.
 
 Many configuration options are available, in order to be able to create
-different rules for different kind of identifier. In general, the
-rules are falling back to a more generic rule if the specific case is not
-configured.
+different rules for different kinds of identifier. In general, the rules are
+falling back to a more generic rule if the specific case is not configured.
+
+Options
+---
+
+.. option:: AbstractClassCase
+
+When defined, the check will ensure abstract class names conform to the
+selected casing.
+
+.. option:: AbstractClassPrefix
+
+When defined, the check will ensure abstract class names will add the
+prefixed with the given value (regardless of casing).
+
+.. option:: AbstractClassSuffix
+
+When defined, the check will ensure abstract class names will add the
+suffix with the given value (regardless of casing).
+
+For example using values of:
+
+   - AbstractClassCase of ``lower_case``
+   - AbstractClassPrefix of ``pre_``
+   - AbstractClassSuffix of ``_post``
+
+Identifies and/or transforms abstract class names as follows:
+
+Before:
+
+.. code-block:: c++
+
+class ABSTRACT_CLASS {
+public:
+  ABSTRACT_CLASS();
+};
+
+After:
+
+.. code-block:: c++
+
+class abstract_class {
+public:
+  pre_abstract_class_post();
+};
+
+.. option:: ClassCase
+
+When defined, the check will ensure class names conform to the
+selected casing.
+
+.. option:: ClassPrefix
+
+When defined, the check will ensure class names will add the
+prefixed with the given value (regardless of casing).
+
+.. option:: ClassSuffix
+
+When defined, the check will ensure class names will add the
+suffix with the given value (regardless of casing).
+
+For example using values of:
+
+   - ClassCase of ``lower_case``
+   - ClassPrefix of ``pre_``
+   - ClassSuffix of ``_post``
+
+Identifies and/or transforms class names as follows:
+
+Before:
+
+.. code-block:: c++
+
+class FOO {
+public:
+  FOO();
+  ~FOO();
+};
+
+After:
+
+.. code-block:: c++
+
+class pre_foo_post {
+public:
+  pre_foo_post();
+  ~pre_foo_post();
+};
+
+.. option:: ClassConstantCase
+
+When defined, the check will ensure class constant names conform to the
+selected casing.
+
+.. option:: ClassConstantPrefix
+
+When defined, the check will ensure class constant names will add the
+prefixed with the given value (regardless of casing).
+
+.. option:: ClassConstantSuffix
+
+When defined, the check will ensure class constant names will add the
+suffix with the given value (regardless of casing).
+
+For example using values of:
+
+   - ClassConstantCase of ``lower_case``
+   - ClassConstantPrefix of ``pre_``
+   - ClassConstantSuffix of ``_post``
+
+Identifies and/or transforms class constant names as follows:
+
+Before:
+
+.. code-block:: c++
+
+class FOO {
+public:
+  static const int CLASS_CONSTANT;
+};
+
+After:
+
+.. code-block:: c++
+
+class FOO {
+public:
+  static const int pre_class_constant_post;
+};
+
+.. option:: ClassMemberCase
+
+When defined, the check will ensure class member names conform to the
+selected casing.
+
+.. option:: ClassMemberPrefix
+
+When defined, the check will ensure class member names will add the
+prefixed with the given value (regardless of casing).
+
+.. option:: ClassMemberSuffix
+
+When defined, the check will ensure class member names will add the
+suffix wi

[PATCH] D56424: [clang-tidy] Add check for underscores in googletest names.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D56424#1356959 , @karepker wrote:

> Hi all, ping on this patch. I've addressed all comments to the best of my 
> ability. Is there anything outstanding that needs to be changed?


Round about this time of a review we normally hear @JonasToth asking if you've 
run this on a large C++ code base like LLVM (with fix-its), and seen if the 
project still builds afterwards..might be worth doing that ahead of time if you 
haven't done so already


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56424



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D55433#1351707 , @JonasToth wrote:

> > I do not have commit rights. I'm not sure what constitutes someone who can 
> > commit, but let me contribute a little more before taking that step,  I 
> > have another idea for a checker I'd like to try after this one, I just 
> > wanted to get one under my belt first.
>
> See this: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access
>
> I will commit for you then. More patches always welcome ;)


@JonasToth, I notice when you commit you commit in 2 places, is this by hand or 
automatic? or if this process is mandated or written down somewhere?

rCTE350760 : [clang-tidy] Adding a new 
modernize use nodiscard checker
rL350760 : [clang-tidy] Adding a new 
modernize use nodiscard checker

Do you have any guidance on how YOU commit (especially in clang-tools-extra) 
that might be useful to others just starting out?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55433



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

[PATCH] D56424: [clang-tidy] Add check for underscores in googletest names.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D56424#1357481 , @lebedev.ri wrote:

> In D56424#1357471 , @MyDeveloperDay 
> wrote:
>
> > In D56424#1356959 , @karepker 
> > wrote:
> >
> > > Hi all, ping on this patch. I've addressed all comments to the best of my 
> > > ability. Is there anything outstanding that needs to be changed?
> >
> >
> > Round about this time of a review we normally hear @JonasToth asking if 
> > you've run this on a large C++ code base like LLVM (with fix-its), and seen 
> > if the project still builds afterwards..might be worth doing that ahead of 
> > time if you haven't done so already
>
>
> From docs: `This check does not propose any fixes.`.


Thats a great suggestion for the future then transform

  TEST(TestCase_Name, Test_Name) {} 

into

  TEST(TestCase_Name, TestName) {}


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56424



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

[PATCH] D56563: [clang-tidy] add options documentation to readability-identifier-naming checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D56563#1358149 , @JonasToth wrote:

>




> Given its length, what do you think about a short link-list at the beginning 
> that will point to the section in the docs? With that its easier to see whats 
> all handled by the check.

How about the following...

F7820384: image.png 


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

https://reviews.llvm.org/D56563



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

[PATCH] D56563: [clang-tidy] add options documentation to readability-identifier-naming checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 181836.
MyDeveloperDay marked 3 inline comments as done.
MyDeveloperDay added a comment.

Addressing review comment


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

https://reviews.llvm.org/D56563

Files:
  docs/clang-tidy/checks/readability-identifier-naming.rst

Index: docs/clang-tidy/checks/readability-identifier-naming.rst
===
--- docs/clang-tidy/checks/readability-identifier-naming.rst
+++ docs/clang-tidy/checks/readability-identifier-naming.rst
@@ -5,14 +5,1919 @@
 
 Checks for identifiers naming style mismatch.
 
-This check will try to enforce coding guidelines on the identifiers naming.
-It supports `lower_case`, `UPPER_CASE`, `camelBack` and `CamelCase` casing and
-tries to convert from one to another if a mismatch is detected.
+This check will try to enforce coding guidelines on the identifiers naming. It
+supports one of the following casing types and tries to convert from one to
+another if a mismatch is detected
 
-It also supports a fixed prefix and suffix that will be prepended or
-appended to the identifiers, regardless of the casing.
+Casing types inclde:
+
+ - ``lower_case``,
+ - ``UPPER_CASE``,
+ - ``camelBack``,
+ - ``CamelCase``,
+ - ``camel_Snake_Back``,
+ - ``Camel_Snake_Case``,
+ - ``aNy_CasE``.
+
+It also supports a fixed prefix and suffix that will be prepended or appended
+to the identifiers, regardless of the casing.
 
 Many configuration options are available, in order to be able to create
-different rules for different kind of identifier. In general, the
-rules are falling back to a more generic rule if the specific case is not
-configured.
+different rules for different kinds of identifiers. In general, the rules are
+falling back to a more generic rule if the specific case is not configured.
+
+Options
+---
+
+The following options are describe below:
+
+ - :option:`AbstractClassCase`, :option:`AbstractClassPrefix`, :option:`AbstractClassSuffix`
+ - :option:`ClassCase`, :option:`ClassPrefix`, :option:`ClassSuffix`
+ - :option:`ClassConstantCase`, :option:`ClassConstantPrefix`, :option:`ClassConstantSuffix`
+ - :option:`ClassMemberCase`, :option:`ClassMemberPrefix`, :option:`ClassMemberSuffix`
+ - :option:`ClassMethodCase`, :option:`ClassMethodPrefix`, :option:`ClassMethodSuffix`
+ - :option:`ConstantCase`, :option:`ConstantPrefix`, :option:`ConstantSuffix`
+ - :option:`ConstantMemberCase`, :option:`ConstantMemberPrefix`, :option:`ConstantMemberSuffix`
+ - :option:`ConstantParameterCase`, :option:`ConstantParameterPrefix`, :option:`ConstantParameterSuffix`
+ - :option:`ConstantPointerParameterCase`, :option:`ConstantPointerParameterPrefix`, :option:`ConstantPointerParameterSuffix`
+ - :option:`ConstexprFunctionCase`, :option:`ConstexprFunctionPrefix`, :option:`ConstexprFunctionSuffix`
+ - :option:`ConstexprMethodCase`, :option:`ConstexprMethodPrefix`, :option:`ConstexprMethodSuffix`
+ - :option:`ConstexprVariableCase`, :option:`ConstexprVariablePrefix`, :option:`ConstexprVariableSuffix`
+ - :option:`EnumCase`, :option:`EnumPrefix`, :option:`EnumSuffix`
+ - :option:`EnumConstantCase`, :option:`EnumConstantPrefix`, :option:`EnumConstantSuffix`
+ - :option:`FunctionCase`, :option:`FunctionPrefix`, :option:`FunctionSuffix`
+ - :option:`GlobalConstantCase`, :option:`GlobalConstantPrefix`, :option:`GlobalConstantSuffix`
+ - :option:`GlobalConstantPointerCase`, :option:`GlobalConstantPointerPrefix`, :option:`GlobalConstantPointerSuffix`
+ - :option:`GlobalFunctionCase`, :option:`GlobalFunctionPrefix`, :option:`GlobalFunctionSuffix`
+ - :option:`GlobalPointerCase`, :option:`GlobalPointerPrefix`, :option:`GlobalPointerSuffix`
+ - :option:`GlobalVariableCase`, :option:`GlobalVariablePrefix`, :option:`GlobalVariableSuffix`
+ - :option:`InlineNamespaceCase`, :option:`InlineNamespacePrefix`, :option:`InlineNamespaceSuffix`
+ - :option:`LocalConstantCase`, :option:`LocalConstantPrefix`, :option:`LocalConstantSuffix`
+ - :option:`LocalConstantPointerCase`, :option:`LocalConstantPointerPrefix`, :option:`LocalConstantPointerSuffix`
+ - :option:`LocalPointerCase`, :option:`LocalPointerPrefix`, :option:`LocalPointerSuffix`
+ - :option:`LocalVariableCase`, :option:`LocalVariablePrefix`, :option:`LocalVariableSuffix`
+ - :option:`MemberCase`, :option:`MemberPrefix`, :option:`MemberSuffix`
+ - :option:`MethodCase`, :option:`MethodPrefix`, :option:`MethodSuffix`
+ - :option:`NamespaceCase`, :option:`NamespacePrefix`, :option:`NamespaceSuffix`
+ - :option:`ParameterCase`, :option:`ParameterPrefix`, :option:`ParameterSuffix`
+ - :option:`ParameterPackCase`, :option:`ParameterPackPrefix`, :option:`ParameterPackSuffix`
+ - :option:`PointerParameterCase`, :option:`PointerParameterPrefix`, :option:`PointerParameterSuffix`
+ - :option:`PrivateMemberCase`, :option:`PrivateMemberPrefix`, :option:`PrivateMemberSuffix`
+ - :option:`PrivateMethodCase`, :option:`PrivateMethodPrefi

[PATCH] D56424: [clang-tidy] Add check for underscores in googletest names.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D56424#1359227 , @karepker wrote:

> In D56424#1357484 , @MyDeveloperDay 
> wrote:
>
> > In D56424#1357481 , @lebedev.ri 
> > wrote:
> >
> > > In D56424#1357471 , 
> > > @MyDeveloperDay wrote:
> > >
> > > > In D56424#1356959 , @karepker 
> > > > wrote:
> > > >
> > > > > Hi all, ping on this patch. I've addressed all comments to the best 
> > > > > of my ability. Is there anything outstanding that needs to be changed?
> > > >
> > > >
> > > > Round about this time of a review we normally hear @JonasToth asking if 
> > > > you've run this on a large C++ code base like LLVM (with fix-its), and 
> > > > seen if the project still builds afterwards..might be worth doing that 
> > > > ahead of time if you haven't done so already
> > >
> > >
> > > From docs: `This check does not propose any fixes.`.
> >
> >
> > Thats a great suggestion for the future then transform
> >
> >   TEST(TestCase_Name, Test_Name) {} 
> >
> >
> > into
> >
> >   TEST(TestCaseName, TestName) {}
> >
>
>
> I considered doing this for the check, but decided against it based on the 
> cases in which I've seen underscores in use. I've seen a few cases in the 
> style of this:
>
> SuccessfullyWrites_InfiniteDeadline
> SuccessfullyWrites_DefaultDeadline
>
> changing these to:
>
> SuccessfullyWritesInfiniteDeadline
> SuccessfullyWritesDefaultDeadline
>
> has a subtle difference to the reader. In the first case, underscore 
> functions like "with", whereas in the fix it sounds like the test is for 
> writing the deadline.
>
> While removing the underscore does seem to work for a large portion of the 
> cases, based on the cases like that above, I didn't think it was appropriate 
> to make these modifications.
>
> Please let me know what you think.


Did I misunderstand I thought the point of the checker was to say "_" in the 
name was illegal? surely the fixit is at liberty to resolve that?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56424



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

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

> LLVM is very chatty as well, I don't consider LLVM to be a bad code-base. 
> Take readability-braces-around-statements for example.

Do we need a llvm-elide-braces-for-small-statements?

This would make a great pre-review check


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54141



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

[PATCH] D56926: [Documentation] Use HTTPS whenever possible in clang-tools-extra

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: 
llvm-svn.src/tools/clang/tools/extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-goto.rst:11
 from the CppCoreGuidelines and 
-`6.3.1 from High Integrity C++ 
`_.
+`6.3.1 from High Integrity C++ 
`_.
 

this one led me to a "You connection is not private" screen

before redirecting me to 
https://www.perforce.com/resources/qac/high-integrity-cpp-coding-standard

{F7828183}


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56926



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

[PATCH] D56926: [Documentation] Use HTTPS whenever possible in clang-tools-extra

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

the closest I can see is

https://www.perforce.com/resources/qac/high-integrity-cpp-coding-standard#statements

which take you to section 6 of the standard, but I see no id or name tags to 
get you to 6.3.1


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56926



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

[PATCH] D56926: [Documentation] Use HTTPS whenever possible in clang-tools-extra

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.

LGTM


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56926



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

[PATCH] D57087: [clang-tidy] add OverrideMacro to modernize-use-override check

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: alexfh, JonasToth, hokein, Eugene.Zelenko, 
aaron.ballman.
MyDeveloperDay added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, xazax.hun.

The usefulness of **modernize-use-override** can be reduced if you have to live 
in an environment where you support multiple compilers, some of which sadly are 
not yet fully C++11 compliant

some codebases have to use override as a macro OVERRIDE e.g.

  #if defined(COMPILER_MSVC)
  #define OVERRIDE override
  #elif defined(__clang__)
  #define OVERRIDE override
  // GCC 4.7 supports explicit virtual overrides when C++11 support is enabled.
  #define OVERRIDE override
  #else
  #define OVERRIDE
  #endif

This allows code to be compiled with C++11 compliant compilers and get warnings 
and errors that clang, MSVC,gcc can give, while still allowing other legacy pre 
C++11 compilers to compile the code. This can be an important step towards 
modernizing C++ code whilst living in a legacy codebase.

When it comes to clang tidy, the use of the **modernize-use-override** is one 
of the most useful checks, but the messages reported are inaccurate for that 
codebase if the standard approach is to use the macros OVERRIDE and/or FINAL.

When combined with fix-its that introduce the C++11 override keyword, they 
become fatal, resulting in the modernize-use-override check being turned off to 
prevent the introduction of such errors.

This revision, allows the possibility for the replacement **override **to be a 
macro instead, Allowing the clang-tidy check to be run on  both pre and post 
C++11 code, and allowing fix-its to be applied.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D57087

Files:
  clang-tidy/modernize/UseOverrideCheck.cpp
  clang-tidy/modernize/UseOverrideCheck.h
  docs/clang-tidy/checks/modernize-use-override.rst
  test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp
  test/clang-tidy/modernize-use-override-cxx98-with-no-macro-inscope.cpp

Index: test/clang-tidy/modernize-use-override-cxx98-with-no-macro-inscope.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-override-cxx98-with-no-macro-inscope.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.OverrideMacro, value: 'CUSTOM_OVERRIDE'},{key: modernize-use-nodiscard.FinalMacro, value: 'FINAL'}]}" \
+// RUN: -- -std=c++98
+
+// As if the macro was not defined.
+//#define CUSTOM_OVERRIDE override
+
+struct Base {
+  virtual ~Base() {}
+  virtual void a();
+  virtual void b();
+};
+
+struct SimpleCases : public Base {
+public:
+  virtual ~SimpleCases();
+  // CHECK-FIXES: {{^}}  virtual ~SimpleCases();
+
+  void a();
+  // CHECK-FIXES: {{^}}  void a();
+
+  virtual void b();
+  // CHECK-FIXES: {{^}}  virtual void b();
+};
Index: test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp
@@ -0,0 +1,95 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.OverrideMacro, value: 'OVERRIDE'},{key: modernize-use-nodiscard.FinalMacro, value: 'FINAL'}]}" \
+// RUN: -- -std=c++98
+
+#define ABSTRACT = 0
+
+#define OVERRIDE override
+#define VIRTUAL virtual
+#define NOT_VIRTUAL
+#define NOT_OVERRIDE
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define UNUSED __attribute__((unused))
+
+struct MUST_USE_RESULT MustUseResultObject {};
+
+struct IntPair {
+  int First, Second;
+};
+
+struct Base {
+  virtual ~Base() {}
+  virtual void a();
+  virtual void b();
+  virtual void c();
+  virtual void d();
+  virtual void d2();
+  virtual void e() = 0;
+  virtual void f() = 0;
+  virtual void f2() const = 0;
+  virtual void g() = 0;
+
+  virtual void j() const;
+  virtual MustUseResultObject k();
+  virtual bool l() MUST_USE_RESULT UNUSED;
+  virtual bool n() MUST_USE_RESULT UNUSED;
+
+  virtual void m();
+  virtual void m2();
+  virtual void o() __attribute__((unused));
+
+  virtual void r() &;
+  virtual void rr() &&;
+
+  virtual void cv() const volatile;
+  virtual void cv2() const volatile;
+
+  virtual void ne() noexcept(false);
+  virtual void t() throw();
+
+  virtual void il(IntPair);
+};
+
+struct SimpleCases : public Base {
+public:
+  virtual ~SimpleCases();
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using 'OVERRIDE' or (rarely) 'FINAL' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  ~SimpleCases() OVERRIDE;
+
+  void a();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: annotate this function with 'OVERRIDE' or (rarely) 'FINAL' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void a() OVERRIDE;
+
+  virtual void b();
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefe

[PATCH] D57087: [clang-tidy] add OverrideMacro to modernize-use-override check

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 183063.
MyDeveloperDay added a comment.

Adding release note change


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

https://reviews.llvm.org/D57087

Files:
  clang-tidy/modernize/UseOverrideCheck.cpp
  clang-tidy/modernize/UseOverrideCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/modernize-use-override.rst
  test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp
  test/clang-tidy/modernize-use-override-cxx98-with-no-macro-inscope.cpp

Index: test/clang-tidy/modernize-use-override-cxx98-with-no-macro-inscope.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-override-cxx98-with-no-macro-inscope.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.OverrideMacro, value: 'CUSTOM_OVERRIDE'},{key: modernize-use-nodiscard.FinalMacro, value: 'FINAL'}]}" \
+// RUN: -- -std=c++98
+
+// As if the macro was not defined.
+//#define CUSTOM_OVERRIDE override
+
+struct Base {
+  virtual ~Base() {}
+  virtual void a();
+  virtual void b();
+};
+
+struct SimpleCases : public Base {
+public:
+  virtual ~SimpleCases();
+  // CHECK-FIXES: {{^}}  virtual ~SimpleCases();
+
+  void a();
+  // CHECK-FIXES: {{^}}  void a();
+
+  virtual void b();
+  // CHECK-FIXES: {{^}}  virtual void b();
+};
Index: test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp
@@ -0,0 +1,95 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.OverrideMacro, value: 'OVERRIDE'},{key: modernize-use-nodiscard.FinalMacro, value: 'FINAL'}]}" \
+// RUN: -- -std=c++98
+
+#define ABSTRACT = 0
+
+#define OVERRIDE override
+#define VIRTUAL virtual
+#define NOT_VIRTUAL
+#define NOT_OVERRIDE
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define UNUSED __attribute__((unused))
+
+struct MUST_USE_RESULT MustUseResultObject {};
+
+struct IntPair {
+  int First, Second;
+};
+
+struct Base {
+  virtual ~Base() {}
+  virtual void a();
+  virtual void b();
+  virtual void c();
+  virtual void d();
+  virtual void d2();
+  virtual void e() = 0;
+  virtual void f() = 0;
+  virtual void f2() const = 0;
+  virtual void g() = 0;
+
+  virtual void j() const;
+  virtual MustUseResultObject k();
+  virtual bool l() MUST_USE_RESULT UNUSED;
+  virtual bool n() MUST_USE_RESULT UNUSED;
+
+  virtual void m();
+  virtual void m2();
+  virtual void o() __attribute__((unused));
+
+  virtual void r() &;
+  virtual void rr() &&;
+
+  virtual void cv() const volatile;
+  virtual void cv2() const volatile;
+
+  virtual void ne() noexcept(false);
+  virtual void t() throw();
+
+  virtual void il(IntPair);
+};
+
+struct SimpleCases : public Base {
+public:
+  virtual ~SimpleCases();
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using 'OVERRIDE' or (rarely) 'FINAL' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  ~SimpleCases() OVERRIDE;
+
+  void a();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: annotate this function with 'OVERRIDE' or (rarely) 'FINAL' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void a() OVERRIDE;
+
+  virtual void b();
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using 'OVERRIDE' or (rarely) 'FINAL' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void b() OVERRIDE;
+
+  virtual void c();
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void c() OVERRIDE;
+
+  virtual void e() = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void e() OVERRIDE = 0;
+
+  virtual void f()=0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void f() OVERRIDE =0;
+
+  virtual void f2() const=0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void f2() const OVERRIDE =0;
+
+  virtual void g() ABSTRACT;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void g() OVERRIDE ABSTRACT;
+
+  virtual void j() const;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void j() const OVERRIDE;
+
+  virtual MustUseResultObject k();  // Has an implicit attribute.
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: prefer using
+  // CHECK-FIXES: {{^}}  MustUseResultObject k() OVERRIDE;
+};
Index: docs/clang-tidy/checks/modernize-use-override.rst
===
--- docs/clang-tidy/checks/modernize-use-override.rst
+++ docs/clang-tidy/checks/modernize-use-override.rst
@@ -3,5 +3,37 @@
 modernize-use-override
 ==
 
+Adds ``override`` (introduced in C++11) to overridden 

[PATCH] D57087: [clang-tidy] add OverrideMacro to modernize-use-override check

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D57087#1367610 , @alexfh wrote:

> I tend to think that a better migration strategy is to change the compiler to 
> a C++11-compatible one first, and then turn on C++11 mode and migrate the 
> code (possibly file-by-file or with a different granularity). But if you 
> observe a situation where compatibility macros for C++11 constructs are 
> actually a better way to migrate, then the proposed functionality makes sense.


@alexfh,  I couldn't agree more, however unfortunately if you are having to 
support a common code base which also supports older platforms like HPUX, 
Solaris, AIX even getting a C++11 compiler can be more of a challenge! Those 
platforms have expensive commercial compilers but are often lacking behind the 
standards.  If your lucky you can find a  gcc that will work but it tends to be 
a low version one. And then building a later gcc on those platforms is often a 
challenge too, even if you can get it and your code to build you often meet 
other hard to find issues with the final binary.

This is similar to the discussion about getting clang to compile with a minimum 
C++14 or C++17 compiler, it can be hard to pull the toolchain up to the level 
where all the supported platforms still work.

Having said all this I appreciate the code review comments, let me take a look 
at rewriting the bits you highlighted...which I totally agree with and think it 
makes for a cleaner solution. (I was trying not alter the code too much around 
line 120)


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

https://reviews.llvm.org/D57087



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

[PATCH] D57087: [clang-tidy] add OverrideMacro to modernize-use-override check

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:32
+: ClangTidyCheck(Name, Context),
+  OverrideMacro(Options.get("OverrideMacro", "override")),
+  FinalMacro(Options.get("FinalMacro", "final")) {}

alexfh wrote:
> I'd suggest to default to an empty string and use `override` as a fallback 
> right in the code where the diagnostic is generated.
So I tried this and and met with some issues with the unit tests where it 
seemed to think "override" was a macro, I also found myself just simply always 
setting OverrideMacro/Final Macro to "override" and "final" anyway.. I've 
changed this round a little to only check for the macro when the OverrideMacro 
isn't override. This seems to resolve the problem, let me know if it still 
feels wrong.



Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:42-46
+  // If we use ``override``, we require at least C++11. Use a
+  // macro with pre c++11 compilers by using OverrideMacro option.
+  if ((OverrideMacro == "override" && !getLangOpts().CPlusPlus11) ||
+  !getLangOpts().CPlusPlus)
+return;

alexfh wrote:
> I think, this should be left as is, because
> 1. clang-tidy understands C++11 and it makes sense to run it in C++11 mode to 
> get more information out of compiler (e.g. then OVERRIDE macro will actually 
> be defined to `override` and clang-tidy wouldn't have to jump through the 
> hoops to detect that a method is already declared `override`).
> 2. I've seen folks who just `#define override` in pre-C++11 mode to make the 
> code compile.
I agree with this..I don't need to run clang-tidy in cxx98 mode..reverting to 
the original check



Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:97
+
+  if (!doesOverrideMacroExist(Context, OverrideMacro))
+return;

alexfh wrote:
> The utility of the function is questionable. I'd drop it and replace the call 
> with `if (!OverrideMacro.empty() && 
> !Context.Idents.get(OverrideMacro).hasMacroDefinition()) ...`.
removed the function, but did change the condition here to be 
OverrideMacro!="override" to overcome issue in the unit test when not 
overriding the macro it would sometimes return here and not perform the fix-it, 
I wondered if  check_clang_tidy.py -fix somehow ran the clang tidy check() 
function twice, once for the message and once for the fix.. but I didn't dig in 
any further.



Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:121-131
+Message = "prefer using '" + OverrideMacro + "' or (rarely) '" +
+  FinalMacro + "' instead of 'virtual'";
   } else if (KeywordCount == 0) {
-Message = "annotate this function with 'override' or (rarely) 'final'";
+Message = "annotate this function with '" + OverrideMacro +
+  "' or (rarely) '" + FinalMacro + "'";
   } else {
 StringRef Redundant =

alexfh wrote:
> How about using diagnostic arguments instead of string concatenation 
> (`diag(Loc, "prefer using %1 or %2") << Macro1 << Macro2;`)?
switched to using %0 and %1 hope that is correct, that 0 vs 1 stumped me for a 
bit, but I looked at other checks doing the same thing



Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:133
+StringRef Correct =
+HasFinal ? "'" + FinalMacro + "'" : "'" + OverrideMacro + "'";
 

JonasToth wrote:
> Dangling? These values seem to be temporary and `StringRef` would bind to the 
> temporary, not?
> For the concatenation `llvm::Twine` would be better as well, same in the 
> other places.
removed the need for so may concatenations, I hope by using %0 and %1



Comment at: test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp:95
+  // CHECK-FIXES: {{^}}  MustUseResultObject k() OVERRIDE;
+};

alexfh wrote:
> Please add a test where the OVERRIDE macro is already present. Same for the 
> FINAL macro.
added a couple more..let me know if its sufficient.. changed the name of the 
test to remove cxx98 too


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

https://reviews.llvm.org/D57087



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

[PATCH] D57087: [clang-tidy] add OverrideMacro to modernize-use-override check

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 183268.
MyDeveloperDay marked 12 inline comments as done.
MyDeveloperDay added a comment.

Addressing review comments,

- reduce changes causing excessive string concats and problems with temporaries
- simplify cxx version and macro checks


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

https://reviews.llvm.org/D57087

Files:
  clang-tidy/modernize/UseOverrideCheck.cpp
  clang-tidy/modernize/UseOverrideCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/modernize-use-override.rst
  test/clang-tidy/modernize-use-override-with-macro.cpp
  test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp

Index: test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.OverrideMacro, value: 'CUSTOM_OVERRIDE'},{key: modernize-use-nodiscard.FinalMacro, value: 'FINAL'}]}" \
+// RUN: -- -std=c++11
+
+// As if the macro was not defined.
+//#define CUSTOM_OVERRIDE override
+
+struct Base {
+  virtual ~Base() {}
+  virtual void a();
+  virtual void b();
+};
+
+struct SimpleCases : public Base {
+public:
+  virtual ~SimpleCases();
+  // CHECK-FIXES: {{^}}  virtual ~SimpleCases();
+
+  void a();
+  // CHECK-FIXES: {{^}}  void a();
+
+  virtual void b();
+  // CHECK-FIXES: {{^}}  virtual void b();
+};
Index: test/clang-tidy/modernize-use-override-with-macro.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-override-with-macro.cpp
@@ -0,0 +1,69 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.OverrideMacro, value: 'OVERRIDE'},{key: modernize-use-nodiscard.FinalMacro, value: 'FINAL'}]}" \
+// RUN: -- -std=c++11
+
+#define ABSTRACT = 0
+
+#define OVERRIDE override
+#define VIRTUAL virtual
+#define NOT_VIRTUAL
+#define NOT_OVERRIDE
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define UNUSED __attribute__((unused))
+
+struct Base {
+  virtual ~Base() {}
+  virtual void a();
+  virtual void b();
+  virtual void c();
+  virtual void e() = 0;
+  virtual void f2() const = 0;
+  virtual void g() = 0;
+  virtual void j() const;
+  virtual void k() = 0;
+  virtual void l() const;
+};
+
+struct SimpleCases : public Base {
+public:
+  virtual ~SimpleCases();
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using 'OVERRIDE' or (rarely) 'FINAL' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  ~SimpleCases() OVERRIDE;
+
+  void a();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: annotate this function with 'OVERRIDE' or (rarely) 'FINAL' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void a() OVERRIDE;
+
+  virtual void b();
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using 'OVERRIDE' or (rarely) 'FINAL' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void b() OVERRIDE;
+
+  virtual void c();
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void c() OVERRIDE;
+
+  virtual void e() = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void e() OVERRIDE = 0;
+
+  virtual void f2() const = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void f2() const OVERRIDE = 0;
+
+  virtual void g() ABSTRACT;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void g() OVERRIDE ABSTRACT;
+
+  virtual void j() const;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void j() const OVERRIDE;
+
+  virtual void k() OVERRIDE;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'OVERRIDE' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void k() OVERRIDE;
+
+  virtual void l() const OVERRIDE;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'OVERRIDE' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void l() const OVERRIDE;
+};
Index: docs/clang-tidy/checks/modernize-use-override.rst
===
--- docs/clang-tidy/checks/modernize-use-override.rst
+++ docs/clang-tidy/checks/modernize-use-override.rst
@@ -3,5 +3,37 @@
 modernize-use-override
 ==
 
+Adds ``override`` (introduced in C++11) to overridden virtual functions and
+removes ``virtual`` from those functions as it is not required.
 
-Use C++11's ``override`` and remove ``virtual`` where applicable.
+virtual on non base class implementations was used to help indiciate to the user
+that a function was virtual. C++ compilers 

[PATCH] D57380: [clang-tools-extra] add missing .clang-format and .clang-tidy for use with git monorepo

[MyDeveloperDay via Phabricator via cfe-commits]

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

Before git monorepo, clang-tools-extra inherited its .clang-format and 
.clang-tidy files from llvm/tools/clang/.clang-format/tidy respectively

When using a git monorepo clang-tools-extra is at the topmost level and there 
is no parental .clang-format/tidy files to inherit (in llvm-project)

This revision simply copies the .clang-format/tidy files from the 
llvm/tools/clang up into llvm/tools/clang/tools/extra so that when its laid out 
as a monorepo, those files are present.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D57380

Files:
  .clang-format
  .clang-tidy


Index: .clang-tidy
===
--- /dev/null
+++ .clang-tidy
@@ -0,0 +1,17 @@
+Checks: 
'-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,readability-identifier-naming'
+CheckOptions:
+  - key: readability-identifier-naming.ClassCase
+value:   CamelCase
+  - key: readability-identifier-naming.EnumCase
+value:   CamelCase
+  - key: readability-identifier-naming.FunctionCase
+value:   camelBack
+  - key: readability-identifier-naming.MemberCase
+value:   CamelCase
+  - key: readability-identifier-naming.ParameterCase
+value:   CamelCase
+  - key: readability-identifier-naming.UnionCase
+value:   CamelCase
+  - key: readability-identifier-naming.VariableCase
+value:   CamelCase
+
Index: .clang-format
===
--- /dev/null
+++ .clang-format
@@ -0,0 +1 @@
+BasedOnStyle: LLVM


Index: .clang-tidy
===
--- /dev/null
+++ .clang-tidy
@@ -0,0 +1,17 @@
+Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,readability-identifier-naming'
+CheckOptions:
+  - key: readability-identifier-naming.ClassCase
+value:   CamelCase
+  - key: readability-identifier-naming.EnumCase
+value:   CamelCase
+  - key: readability-identifier-naming.FunctionCase
+value:   camelBack
+  - key: readability-identifier-naming.MemberCase
+value:   CamelCase
+  - key: readability-identifier-naming.ParameterCase
+value:   CamelCase
+  - key: readability-identifier-naming.UnionCase
+value:   CamelCase
+  - key: readability-identifier-naming.VariableCase
+value:   CamelCase
+
Index: .clang-format
===
--- /dev/null
+++ .clang-format
@@ -0,0 +1 @@
+BasedOnStyle: LLVM
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I checked this out and built this and it works really well. The thing I really 
like is that it helps to format Windows resource include files e.g. 
'Resource.h'   better than before.

Which changes the current clang-formatted version

  #define IDD_ 101
  #define IDS_BBB 101
  #define IDR_CCC 102
  #define IDB_ 103
  #define IDS_EE 104
  #define IDR_ 106
  #define IDD_GG 107
  #define IDD_H 108
  #define IDS_I 111
  #define IDS_ 112
  #define IDS_KKK 114
  #define IDD_ 115

to

  #define IDD_   101
  #define IDS_BBB101
  #define IDR_CCC102
  #define IDB_   103
  #define IDS_EE 104
  #define IDR_   106
  #define IDD_GG 107
  #define IDD_H  108
  #define IDS_I  111
  #define IDS_   112
  #define IDS_KKK114
  #define IDD_   115

But alas it will still fight with Visual Studio which wants to align them with 
its 40 characters or 1+ more than the #define length strategy (e.g.)

  #define IDD_101
  #define IDS_BBB 101
  #define IDR_CCC 102
  #define IDB_ 103
  #define IDS_EE  104
  #define IDR_ 106
  #define IDD_GG 107
  #define IDD_H 108
  #define IDS_I   111
  #define IDS_112
  #define IDS_KKK 114
  #define IDD_115

I don't want to suggest anything that would block this patch going in, ( 
because I know people have been waiting for it for a long time) , but 
ultimately I wonder if people may suggest having different macro alignment 
strategies and options later on.

Maybe if AlignConsecutiveMacros was an enumeration rather than just a Boolean 
(even if its only had None,Aligned for now)  it might allow someone to more 
easily add a new strategy later on without breaking compatibility.

Apart form that I think its great, and thank you for taking the mantle.


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

https://reviews.llvm.org/D28462



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

[PATCH] D57380: [clang-tools-extra] add missing .clang-format and .clang-tidy for use with git monorepo

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay abandoned this revision.
MyDeveloperDay added a comment.

Abandoning as its been superseded by...

https://github.com/llvm/llvm-project/commit/149be18dbc4d54328fe33b5ac21dcbbca2b07aa6

Which puts these files at the base of llvm-project


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57380



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

[PATCH] D57353: [clang-tidy] Add the abseil-duration-double-conversion check

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: docs/clang-tidy/checks/abseil-duration-double-conversion.rst:20
+
+
+  // Original - Conversion to integer and back again

hwright wrote:
> Eugene.Zelenko wrote:
> > Unnecessary empty line.
> This is consistent with other documentation in this directory, such as 
> `abseil-faster-strsplit-delimiter.rst`.
In your example `abseil-faster-strsplit-delimiter.rst` , The double blank line 
in the html doesn't give much delineation between the before and after code and 
the next example.

{F7867869}

There probably isn't a convention per say (which is a shame), across the docs 
we do a mixture of different styles 

https://clang.llvm.org/extra/clang-tidy/checks/readability-braces-around-statements.html
https://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-accept.html
https://clang.llvm.org/extra/clang-tidy/checks/google-objc-function-naming.html

But there is a desire by some of the regular clang-tidy reviewers to make the 
documentation consistent

It may not be ideal but the "Before/After" style, that is used in 
`modernize-use-emplace`, 
`modernize-use-using`,`readability-braces-around-statements`,`readability-identifier-naming`
 and `readability-redundant-function-ptr-dereference` does help a little.

I'm not saying looks better, but I've added a couple of examples of formatting 
the strsplit example for comparison, feel free to ignore.

{F7867960}

{F7868028}




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

https://reviews.llvm.org/D57353



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

[PATCH] D57435: [clang-tidy] Add abseil-wrap-unique check

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/abseil/WrapUniqueCheck.cpp:27
+
+  return (ArgRef.str().length() > 0) ? ArgRef.str() + ")" : "()";
+}

couldn't this be 

```
return (!ArgRef.empty()) ? ArgRef.str() + ")" : "()";
```

You know I think there is a clang-tidy check for using empty() which was 
recently extended to handle length() too... I wonder if that would have fired



Comment at: clang-tidy/abseil/WrapUniqueCheck.cpp:62
+std::string diagText = "Perfer absl::WrapUnique for resetting unique_ptr";
+std::string newText;
+

can we declare this when we create it see below?



Comment at: clang-tidy/abseil/WrapUniqueCheck.cpp:72
+
+newText =
+ObjName.str() + " = absl::WrapUnique(" + getArgs(SM, callExpr) + ")";

std::string newText = ..



Comment at: clang-tidy/abseil/WrapUniqueCheck.cpp:84
+
+std::string diagText = "Perfer absl::WrapUnique to constructing 
unique_ptr";
+std::string newText;

clang-tidy file in llvm/tools/clang says

key: readability-identifier-naming.VariableCase
value:   CamelCase

this means that local variables like this should be DiagText and NewText (I 
think!)



Comment at: clang-tidy/abseil/WrapUniqueCheck.cpp:85
+std::string diagText = "Perfer absl::WrapUnique to constructing 
unique_ptr";
+std::string newText;
+std::string Left;

remove empty declarations until they are created



Comment at: clang-tidy/abseil/WrapUniqueCheck.cpp:93
+
+Left = (consDecl) ? "auto " + NameRef.str() + " = " : "";
+newText = Left + "absl::WrapUnique(" + getArgs(SM, FC_Call) + ")";

std::string Left = (cons..



Comment at: clang-tidy/abseil/WrapUniqueCheck.cpp:94
+Left = (consDecl) ? "auto " + NameRef.str() + " = " : "";
+newText = Left + "absl::WrapUnique(" + getArgs(SM, FC_Call) + ")";
+SourceLocation Target =

std::string newText = 



Comment at: docs/ReleaseNotes.rst:83
+
+
 Improvements to include-fixer

remove double bank line 


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57435



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

[PATCH] D57435: [clang-tidy] Add abseil-wrap-unique check

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/abseil/WrapUniqueCheck.cpp:54
+  const auto *facExpr = Result.Nodes.getNodeAs("facCons");
+  const auto *callExpr = Result.Nodes.getNodeAs("callExpr");
+

I'm not sure how expensive getNodeAs is... or if the convention is to do them 
all at the top of the function

but couldn't some of these getNodeAs go inside the scope where they are used

if (facExpr){
  const auto *callExpr = 



Comment at: clang-tidy/abseil/WrapUniqueCheck.cpp:59
+  const auto *FC_Call = Result.Nodes.getNodeAs("FC_call");
+
+  if (facExpr) {

same with the 2 above they are only used in the if (cons) scope


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57435



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

[PATCH] D57435: [clang-tidy] Add abseil-wrap-unique check

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/abseil/WrapUniqueCheck.h:27
+private:
+  std::string getArgs(const SourceManager *SM, const CallExpr *MemExpr);
+

Nit: could this method could be static in the cpp file and not in the header?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57435



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

[PATCH] D57353: [clang-tidy] Add the abseil-duration-double-conversion check

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: docs/clang-tidy/checks/abseil-duration-double-conversion.rst:20
+
+
+  // Original - Conversion to integer and back again

hwright wrote:
> MyDeveloperDay wrote:
> > hwright wrote:
> > > Eugene.Zelenko wrote:
> > > > Unnecessary empty line.
> > > This is consistent with other documentation in this directory, such as 
> > > `abseil-faster-strsplit-delimiter.rst`.
> > In your example `abseil-faster-strsplit-delimiter.rst` , The double blank 
> > line in the html doesn't give much delineation between the before and after 
> > code and the next example.
> > 
> > {F7867869}
> > 
> > There probably isn't a convention per say (which is a shame), across the 
> > docs we do a mixture of different styles 
> > 
> > https://clang.llvm.org/extra/clang-tidy/checks/readability-braces-around-statements.html
> > https://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-accept.html
> > https://clang.llvm.org/extra/clang-tidy/checks/google-objc-function-naming.html
> > 
> > But there is a desire by some of the regular clang-tidy reviewers to make 
> > the documentation consistent
> > 
> > It may not be ideal but the "Before/After" style, that is used in 
> > `modernize-use-emplace`, 
> > `modernize-use-using`,`readability-braces-around-statements`,`readability-identifier-naming`
> >  and `readability-redundant-function-ptr-dereference` does help a little.
> > 
> > I'm not saying looks better, but I've added a couple of examples of 
> > formatting the strsplit example for comparison, feel free to ignore.
> > 
> > {F7867960}
> > 
> > {F7868028}
> > 
> > 
> I like those examples!
> 
> Would it be reasonable to update all of the `abseil-duration-*` documentation 
> in a separate pass, after this change is submitted?
If it means the documentation gets better in the long run, I'm fine with that, 
its mainly a Nit on my part.


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

https://reviews.llvm.org/D57353



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

[PATCH] D57108: [clang-tidy] diagnose possibiltiy to add 'noexcept' in modernize-use-noexcept

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/modernize/UseNoexceptCheck.h:1
 //===--- UseNoexceptCheck.h - clang-tidy-*- C++ 
-*-===//
 //

Nit: sorry OCD kicking in... space after tidy and before first -


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57108



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

[PATCH] D57435: [clang-tidy] Add abseil-wrap-unique check

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

This is the clang-tidy revision that mgiht have caught the str().length() > 0  
case D56644: [clang-tidy] readability-container-size-empty handle std::string 
length()   adding a a cross reference for that 
work.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57435



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

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: test/clang-tidy/readability-container-size-empty.cpp:135
+  // CHECK-FIXES: {{^  }}if (str.empty()){{$}}
+  if (str.length() == 0)
+;

could you add a  test that checks if StringRef.str().length() >0 becomes 
!StringRef.str.empty()

e.g. 


```
LLVM::StringRef ArgRef;

return (ArgRef.str().length() > 0) ? ArgRef.str() + ")" : "()";
```


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56644



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D55433#1323106 , @lebedev.ri wrote:

> Have you evaluated this on some major C++ projects yet?
>  I suspect this may have pretty low SNR.


Internally yes (on millions of lines), but you are correct high noise level, 
however resulting compiles showed a number of hard to find bugs most notably 
where empty()  function on custom container was being used when they meant 
clear(), this bug had gone undetected for 10+ years.

The idea came from guidance given in Jason Turner CppCon2018 talk, 
https://www.youtube.com/watch?v=DHOlsEd0eDE (about 14 minutes in)

"is it a logical error in your code to call this member function and discard 
the return value"... the room says yes!!


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55433



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I'm trying to generate an example of the modernize-use-nodiscard checker on 
something open source...as I developed this checker on Windows I struggled a 
little getting cmake to build me the json file to run clang-tidy over 
everything and a lot of projects aren't setup of c++17 yet which is needed for 
[[nodiscard]] to be allowed,  but using ClangPowerTools inside Visual studio I 
decided to run it on **protobuf**

I ran "Tidy Fix" with just the following custom checks 
"-*,modernize-use-nodiscard" this will fix some of the headers (alot of the 
headers actually)

recompiling the project, I see this one, which made me laugh...

  protobuf\protobuf\src\google\protobuf\descriptor.cc(4238): warning C4834: 
discarding return value of function with 'nodiscard' attribute

from descriptor.h I can see its fixed the header adding [[nodiscard]]'s

  // Tries to find something in the fallback database and link in the
  // corresponding proto file.  Returns true if successful, in which case
  // the caller should search for the thing again.  These are declared
  // const because they are called by (semantically) const methods.
  [[nodiscard]] bool TryFindFileInFallbackDatabase(const std::string& name) 
const;
  [[nodiscard]] bool TryFindSymbolInFallbackDatabase(const std::string& name) 
const;

ironically there is a comment in the code...

if (tables_->FindFile(proto.dependency(i)) == NULL &&
(pool_->underlay_ == NULL ||
 pool_->underlay_->FindFileByName(proto.dependency(i)) == NULL)) {
  // We don't care what this returns since we'll find out below anyway.
  pool_->TryFindFileInFallbackDatabase(proto.dependency(i));
  ^^
}
  }
  tables_->pending_files_.pop_back();

For me its as much about why the comment was put there, someone was having to 
explain why they didn't care about the return, but this could easily have been 
a bug

Bug the clang-tidy -fix doesn't break the build, the patch is large but nothing 
broke.

F7661182: protobuf.patch 


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55433



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177295.
MyDeveloperDay added a comment.

Address some (not all yet) review comments


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

https://reviews.llvm.org/D55433

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseNodiscardCheck.cpp
  clang-tidy/modernize/UseNodiscardCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-nodiscard.rst
  test/clang-tidy/modernize-use-nodiscard.cpp

Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,138 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+
+// TODO using macros to avoid CHECK-FIXES not being able to find [[xxx]]
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
+class Foo
+{
+public:
+bool f1() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f1() const;
+
+bool f2(int) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f2(int) const;
+
+bool f3(const int &) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f3(const int &) const;
+
+bool f4(void) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f4(void) const;
+
+// negative tests
+
+void f5() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: void f5() const;
+
+bool f6();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f6();
+
+bool f7(int &);
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f7(int &);
+
+bool f8(int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f8(int &) const;
+
+bool f9(int *) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f9(int *) const;
+
+bool f10(const int &,int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f10(const int &,int &) const;
+
+NO_DISCARD bool f12() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_DISCARD bool f12() const;
+
+MUST_USE_RESULT bool f13() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: MUST_USE_RESULT bool f13() const;
+   
+// TODO fix CHECK-FIXES to handle "[[" "]]"
+[[nodiscard]] bool f11() const;
+// CHECK-MESSAGES-NOT: warning:
+// _CHECK-FIXES: [[nodiscard]] bool f11() const;
+
+bool _f20() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool _f20() const;
+
+NO_RETURN bool f21() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_RETURN bool f21() const;
+
+~Foo();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: ~Foo();
+
+bool operator +=(int) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool operator +=(int) const;
+   
+// extra keywords (virtual,inline,const) on return type
+
+virtual bool f14() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f14' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD virtual bool f14() const;
+
+const bool f15() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f15' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD const bool f15() const;
+
+inline const bool f16() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f16' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const bool f16() const;
+
+inline virtual const bool f17() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f17' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline virtual const bool f17() const;
+
+// inline with body
+bool f18() const {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f18' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f18() const {
+return true;
+}
+
+bool f19() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f19' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f19() const;
+};
+
+bool Foo::f19() const
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 12 inline comments as done.
MyDeveloperDay added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:28-30
+bool isOperator(const FunctionDecl *D) {
+  return D->getNameAsString().find("operator") != std::string::npos;
+}

lebedev.ri wrote:
> Can't you use `isOverloadedOperator()`?
> No way going to `std::string` is fast.
Great! I didn't even know that existed (this is my first checker so I'm still 
learning what I can do), I tried to use it in the matcher but I couldn't get it 
to work, (see comment below about your matcher comment)



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:32-34
+bool isInternalFunction(const FunctionDecl *D) {
+  return D->getNameAsString().find("_") == 0;
+}

lebedev.ri wrote:
> Motivation?
> This should be configurable in some way.
this was because when I ran it on my source tree with -fix it, it wanted to try 
and fix the windows headers, and that scared me a bit! so I was trying to 
prevent it stepping into standard headers etc.. I'll make a change to make that 
optional



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:47
+void UseNodiscardCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus17)
+return;

lebedev.ri wrote:
> This should be smarter, since you provide `ReplacementString`.
> E.g. if `ReplacementString` is default, require C++17.
> Else, only require C++.
That's a great idea, my team can barely get onto all of C++ 11 because some 
platform compilers are SO far behind, but I really like that idea of allowing 
it to still work if using a macro



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:63-95
+  // if the location is invalid forget it
+  if (!MatchedDecl->getLocation().isValid())
+return;
+
+  if (MatchedDecl->isThisDeclarationADefinition() && 
MatchedDecl->isOutOfLine())
+return;
+

lebedev.ri wrote:
> All this should be done in `registerMatchers()`.
Ok, I think this is my naivety on how to actually do it, because I tried but 
often the functions on MatchDecl->isXXX() didn't always map 1-to-1 with what 
the matchers were expecting (but most likely I was just doing it wrong), let me 
swing back round once I've read some more from @stephenkelly blogs



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:108
+  << MatchedDecl
+  << FixItHint::CreateInsertion(retLoc, NoDiscardMacro + " ");
+  return;

lebedev.ri wrote:
> Can you actually provide fix-it though?
> If the result will be unused, it will break build under `-Werror`.
It is true, it will generate warnings if people aren't using it, does that go 
against what people traditionally believe clang-tidy should do? I mean I get it 
that clang-tidy should mostly tidy the code and leave it in a working state, 
but is causing people to break their eggs considered a no-no? again this might 
be my naivety about what clang-tidy remit is.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:6
+
+This check adds ``[[nodiscard]]`` attributes (introduced in C++17) to member 
functions to highlight at compile time where the return value of a function 
should not be ignored
+

lebedev.ri wrote:
> Eugene.Zelenko wrote:
> > Please use 80 characters limit.
> Same, the rules need to be spelled out, this sounds a bit too magical right 
> now,
I'm not a great wordsmith..but is this too wordy for what you like in the 
Docs/ReleaseNotes?


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

https://reviews.llvm.org/D55433



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177311.
MyDeveloperDay marked 3 inline comments as done.
MyDeveloperDay added a comment.

Move more of the conditional checks into the matchers


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

https://reviews.llvm.org/D55433

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseNodiscardCheck.cpp
  clang-tidy/modernize/UseNodiscardCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-nodiscard.rst
  test/clang-tidy/modernize-use-nodiscard.cpp

Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,138 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+
+// TODO using macros to avoid CHECK-FIXES not being able to find [[xxx]]
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
+class Foo
+{
+public:
+bool f1() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f1() const;
+
+bool f2(int) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f2(int) const;
+
+bool f3(const int &) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f3(const int &) const;
+
+bool f4(void) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f4(void) const;
+
+// negative tests
+
+void f5() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: void f5() const;
+
+bool f6();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f6();
+
+bool f7(int &);
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f7(int &);
+
+bool f8(int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f8(int &) const;
+
+bool f9(int *) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f9(int *) const;
+
+bool f10(const int &,int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f10(const int &,int &) const;
+
+NO_DISCARD bool f12() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_DISCARD bool f12() const;
+
+MUST_USE_RESULT bool f13() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: MUST_USE_RESULT bool f13() const;
+   
+// TODO fix CHECK-FIXES to handle "[[" "]]"
+[[nodiscard]] bool f11() const;
+// CHECK-MESSAGES-NOT: warning:
+// _CHECK-FIXES: [[nodiscard]] bool f11() const;
+
+bool _f20() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool _f20() const;
+
+NO_RETURN bool f21() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_RETURN bool f21() const;
+
+~Foo();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: ~Foo();
+
+bool operator +=(int) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool operator +=(int) const;
+   
+// extra keywords (virtual,inline,const) on return type
+
+virtual bool f14() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f14' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD virtual bool f14() const;
+
+const bool f15() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f15' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD const bool f15() const;
+
+inline const bool f16() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f16' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const bool f16() const;
+
+inline virtual const bool f17() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f17' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline virtual const bool f17() const;
+
+// inline with body
+bool f18() const {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f18' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f18() const {
+return true;
+}
+
+bool f19() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f19' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f19() const;
+};
+
+bool Foo::f19() co

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177312.
MyDeveloperDay added a comment.

Move the conditional checks into matchers


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

https://reviews.llvm.org/D55433

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseNodiscardCheck.cpp
  clang-tidy/modernize/UseNodiscardCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-nodiscard.rst
  test/clang-tidy/modernize-use-nodiscard.cpp

Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,138 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+
+// TODO using macros to avoid CHECK-FIXES not being able to find [[xxx]]
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
+class Foo
+{
+public:
+bool f1() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f1() const;
+
+bool f2(int) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f2(int) const;
+
+bool f3(const int &) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f3(const int &) const;
+
+bool f4(void) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f4(void) const;
+
+// negative tests
+
+void f5() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: void f5() const;
+
+bool f6();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f6();
+
+bool f7(int &);
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f7(int &);
+
+bool f8(int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f8(int &) const;
+
+bool f9(int *) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f9(int *) const;
+
+bool f10(const int &,int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f10(const int &,int &) const;
+
+NO_DISCARD bool f12() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_DISCARD bool f12() const;
+
+MUST_USE_RESULT bool f13() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: MUST_USE_RESULT bool f13() const;
+   
+// TODO fix CHECK-FIXES to handle "[[" "]]"
+[[nodiscard]] bool f11() const;
+// CHECK-MESSAGES-NOT: warning:
+// _CHECK-FIXES: [[nodiscard]] bool f11() const;
+
+bool _f20() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool _f20() const;
+
+NO_RETURN bool f21() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_RETURN bool f21() const;
+
+~Foo();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: ~Foo();
+
+bool operator +=(int) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool operator +=(int) const;
+   
+// extra keywords (virtual,inline,const) on return type
+
+virtual bool f14() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f14' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD virtual bool f14() const;
+
+const bool f15() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f15' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD const bool f15() const;
+
+inline const bool f16() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f16' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const bool f16() const;
+
+inline virtual const bool f17() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f17' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline virtual const bool f17() const;
+
+// inline with body
+bool f18() const {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f18' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f18() const {
+return true;
+}
+
+bool f19() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f19' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f19() const;
+};
+
+bool Foo::f19() const
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES:

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177313.
MyDeveloperDay added a comment.

Move the conditional statements into AST_MATCHERS


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

https://reviews.llvm.org/D55433

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseNodiscardCheck.cpp
  clang-tidy/modernize/UseNodiscardCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-nodiscard.rst
  test/clang-tidy/modernize-use-nodiscard.cpp

Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,138 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+
+// TODO using macros to avoid CHECK-FIXES not being able to find [[xxx]]
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
+class Foo
+{
+public:
+bool f1() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f1() const;
+
+bool f2(int) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f2(int) const;
+
+bool f3(const int &) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f3(const int &) const;
+
+bool f4(void) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f4(void) const;
+
+// negative tests
+
+void f5() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: void f5() const;
+
+bool f6();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f6();
+
+bool f7(int &);
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f7(int &);
+
+bool f8(int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f8(int &) const;
+
+bool f9(int *) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f9(int *) const;
+
+bool f10(const int &,int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f10(const int &,int &) const;
+
+NO_DISCARD bool f12() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_DISCARD bool f12() const;
+
+MUST_USE_RESULT bool f13() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: MUST_USE_RESULT bool f13() const;
+   
+// TODO fix CHECK-FIXES to handle "[[" "]]"
+[[nodiscard]] bool f11() const;
+// CHECK-MESSAGES-NOT: warning:
+// _CHECK-FIXES: [[nodiscard]] bool f11() const;
+
+bool _f20() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool _f20() const;
+
+NO_RETURN bool f21() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_RETURN bool f21() const;
+
+~Foo();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: ~Foo();
+
+bool operator +=(int) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool operator +=(int) const;
+   
+// extra keywords (virtual,inline,const) on return type
+
+virtual bool f14() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f14' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD virtual bool f14() const;
+
+const bool f15() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f15' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD const bool f15() const;
+
+inline const bool f16() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f16' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const bool f16() const;
+
+inline virtual const bool f17() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f17' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline virtual const bool f17() const;
+
+// inline with body
+bool f18() const {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f18' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f18() const {
+return true;
+}
+
+bool f19() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f19' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f19() const;
+};
+
+bool Foo::f19() const
+// CHECK-MESSAGES-NOT: warning:
+// CHEC

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177327.
MyDeveloperDay added a comment.

- Move the final conditional statements into AST_MATCHERS
- Add additional IgnoreInternalFunctions option to allow adding of 
``[[nodiscard]]`` to functions starting with _ (e.g. _Foo())


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

https://reviews.llvm.org/D55433

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseNodiscardCheck.cpp
  clang-tidy/modernize/UseNodiscardCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-nodiscard.rst
  test/clang-tidy/modernize-use-nodiscard.cpp

Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,138 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+
+// TODO using macros to avoid CHECK-FIXES not being able to find [[xxx]]
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
+class Foo
+{
+public:
+bool f1() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f1() const;
+
+bool f2(int) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f2(int) const;
+
+bool f3(const int &) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f3(const int &) const;
+
+bool f4(void) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f4(void) const;
+
+// negative tests
+
+void f5() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: void f5() const;
+
+bool f6();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f6();
+
+bool f7(int &);
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f7(int &);
+
+bool f8(int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f8(int &) const;
+
+bool f9(int *) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f9(int *) const;
+
+bool f10(const int &,int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f10(const int &,int &) const;
+
+NO_DISCARD bool f12() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_DISCARD bool f12() const;
+
+MUST_USE_RESULT bool f13() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: MUST_USE_RESULT bool f13() const;
+   
+// TODO fix CHECK-FIXES to handle "[[" "]]"
+[[nodiscard]] bool f11() const;
+// CHECK-MESSAGES-NOT: warning:
+// _CHECK-FIXES: [[nodiscard]] bool f11() const;
+
+bool _f20() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool _f20() const;
+
+NO_RETURN bool f21() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_RETURN bool f21() const;
+
+~Foo();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: ~Foo();
+
+bool operator +=(int) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool operator +=(int) const;
+   
+// extra keywords (virtual,inline,const) on return type
+
+virtual bool f14() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f14' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD virtual bool f14() const;
+
+const bool f15() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f15' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD const bool f15() const;
+
+inline const bool f16() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f16' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const bool f16() const;
+
+inline virtual const bool f17() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f17' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline virtual const bool f17() const;
+
+// inline with body
+bool f18() const {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f18' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f18() const {
+return true;
+}
+
+bool f19() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f19' should be marked NO_DISCARD [modernize-use-no

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177330.
MyDeveloperDay added a comment.

Minor alterations to Release notes and Documentation to address review comments


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

https://reviews.llvm.org/D55433

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseNodiscardCheck.cpp
  clang-tidy/modernize/UseNodiscardCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-nodiscard.rst
  test/clang-tidy/modernize-use-nodiscard.cpp

Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,138 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+
+// TODO using macros to avoid CHECK-FIXES not being able to find [[xxx]]
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
+class Foo
+{
+public:
+bool f1() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f1() const;
+
+bool f2(int) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f2(int) const;
+
+bool f3(const int &) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f3(const int &) const;
+
+bool f4(void) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f4(void) const;
+
+// negative tests
+
+void f5() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: void f5() const;
+
+bool f6();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f6();
+
+bool f7(int &);
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f7(int &);
+
+bool f8(int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f8(int &) const;
+
+bool f9(int *) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f9(int *) const;
+
+bool f10(const int &,int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f10(const int &,int &) const;
+
+NO_DISCARD bool f12() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_DISCARD bool f12() const;
+
+MUST_USE_RESULT bool f13() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: MUST_USE_RESULT bool f13() const;
+   
+// TODO fix CHECK-FIXES to handle "[[" "]]"
+[[nodiscard]] bool f11() const;
+// CHECK-MESSAGES-NOT: warning:
+// _CHECK-FIXES: [[nodiscard]] bool f11() const;
+
+bool _f20() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool _f20() const;
+
+NO_RETURN bool f21() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_RETURN bool f21() const;
+
+~Foo();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: ~Foo();
+
+bool operator +=(int) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool operator +=(int) const;
+   
+// extra keywords (virtual,inline,const) on return type
+
+virtual bool f14() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f14' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD virtual bool f14() const;
+
+const bool f15() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f15' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD const bool f15() const;
+
+inline const bool f16() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f16' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const bool f16() const;
+
+inline virtual const bool f17() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f17' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline virtual const bool f17() const;
+
+// inline with body
+bool f18() const {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f18' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f18() const {
+return true;
+}
+
+bool f19() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f19' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f19() const;
+};
+
+bool Foo::f19() const
+// CHECK-MESSA

[PATCH] D55482: [clang-tidy] Improve google-objc-function-naming diagnostics 📙

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: test/clang-tidy/google-objc-function-naming.m:8
+// must be in Pascal case as required by Google Objective-C style guide
 // CHECK-FIXES: static bool Ispositive(int a) { return a > 0; }
 

I realize there are words that begin with 'is...' but you could detect if the 
function returned a boolean and started with "is,has,does" and could this 
extrapolate to  IsPositive()  HasSomething(), DoesHaveSomething(),  this might 
generate a better fixit candidate? (just a suggestion feel free to ignore)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55482



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177474.
MyDeveloperDay marked 11 inline comments as done.
MyDeveloperDay added a comment.

- Addressing review comments and concerns
- Removed internal function logic and option, not really the role of this 
checker
- Fixed grammatical error in documentation
- enabled fix-it check in unit tests for  already present [[nodiscard]] by 
using escaping which allowed use of [[]] in CHECK-FIXES
- minor clang format issue


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

https://reviews.llvm.org/D55433

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseNodiscardCheck.cpp
  clang-tidy/modernize/UseNodiscardCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-nodiscard.rst
  test/clang-tidy/modernize-use-nodiscard.cpp

Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,138 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+
+// TODO using macros to avoid CHECK-FIXES not being able to find [[xxx]]
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
+class Foo
+{
+public:
+bool f1() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f1() const;
+
+bool f2(int) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f2(int) const;
+
+bool f3(const int &) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f3(const int &) const;
+
+bool f4(void) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f4(void) const;
+
+// negative tests
+
+void f5() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: void f5() const;
+
+bool f6();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f6();
+
+bool f7(int &);
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f7(int &);
+
+bool f8(int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f8(int &) const;
+
+bool f9(int *) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f9(int *) const;
+
+bool f10(const int &,int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f10(const int &,int &) const;
+
+NO_DISCARD bool f12() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_DISCARD bool f12() const;
+
+MUST_USE_RESULT bool f13() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: MUST_USE_RESULT bool f13() const;
+   
+// TODO fix CHECK-FIXES to handle "[[" "]]"
+[[nodiscard]] bool f11() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: {{\[\[nodiscard\]\]}} bool f11() const;
+
+bool _f20() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function '_f20' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool _f20() const;
+
+NO_RETURN bool f21() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_RETURN bool f21() const;
+
+~Foo();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: ~Foo();
+
+bool operator +=(int) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool operator +=(int) const;
+   
+// extra keywords (virtual,inline,const) on return type
+
+virtual bool f14() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f14' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD virtual bool f14() const;
+
+const bool f15() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f15' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD const bool f15() const;
+
+inline const bool f16() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f16' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const bool f16() const;
+
+inline virtual const bool f17() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f17' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline virtual const bool f17() const;
+
+// inline with body
+bool f18() const {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warnin

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:43
+  // function like _Foo()
+  if (ignore){
+ return doesFunctionNameStartWithUnderScore(&Node);

curdeius wrote:
> If think that you should run clang-format over your patch.
Sorry my bad, I do have clang-format to run on save which should pick up your 
style, but I forgot you guys don't like braces on small ifs...



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:62
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()
+

curdeius wrote:
> curdeius wrote:
> > Missing leading `[` in `[nodiscard]]`, should be `[[nodiscard]]`
> It might have been discussed already, but is it a common convention to mark 
> internal functions with a leading underscore?
> If that comes from system headers, AFAIK clang-tidy already is able not to 
> emit warnings from them.
If you feel this is an unnecessary option, I'm happy to remove it, it might 
simplify things.


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

https://reviews.llvm.org/D55433



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

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



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:62
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()
+

MyDeveloperDay wrote:
> curdeius wrote:
> > curdeius wrote:
> > > Missing leading `[` in `[nodiscard]]`, should be `[[nodiscard]]`
> > It might have been discussed already, but is it a common convention to mark 
> > internal functions with a leading underscore?
> > If that comes from system headers, AFAIK clang-tidy already is able not to 
> > emit warnings from them.
> If you feel this is an unnecessary option, I'm happy to remove it, it might 
> simplify things.
on reflection as both @curdeius  and other reviews expressed concern of 
"isInternalFunction" motivation, I decided to remove this as an option, I think 
I was letting my usecase get mixed up with the design


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

https://reviews.llvm.org/D55433



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D55433#1325117 , @lebedev.ri wrote:

> In D55433#1323779 , @lebedev.ri 
> wrote:
>
> > In D55433#1323757 , 
> > @MyDeveloperDay wrote:
> >
> > > a lot of projects aren't setup for c++17 yet which is needed for 
> > > [[nodiscard]] to be allowed,
> >
> >
> > You can use `[[clang::warn_unused_result]]` for this evaluation, that does 
> > not require C++17.
> >
> > > But the clang-tidy -fix doesn't break the build, the patch is large but 
> > > nothing broke. (at compile time at least!)
> > > 
> > >   {F7661182} 
> >
> > Uhm, so there wasn't a single true-positive in protobuf?
>
>
> To elaborate, what i'm asking is:
>
> - Is that project -Werror clean without those changes?
> - if yes, after all those changes, does the -Werror build break?
>   - If it does break, how many of those issues are actual bugs (ignored 
> return when it should not be ignored), and how many are noise.
>   - If not, then i guess all these were "false-positives"


I totally get where you are coming from, and I want to answer correctly...

1. protobuf was clean to begin with when compiling with -Werror
2. after applying nodiscard fix-it protobuf would not build cleanly with 
-Werror but will build cleanly without it
3. those warnigns ARE related to the introduction of the [[nodiscard]]

  F7673134: image.png 

taking the first one as an example, I'm trying to determine if

  int ExtensionSet::NumExtensions() const {
int result = 0;
ForEach([&result](int /* number */, const Extension& ext) {
  ^^^
  if (!ext.is_cleared) {
++result;
  }
});
return result;
  }

having the checker added nodiscard..

  template 
  [[nodiscard]] KeyValueFunctor ForEach(KeyValueFunctor func) const {
if (PROTOBUF_PREDICT_FALSE(is_large())) {
  return ForEach(map_.large->begin(), map_.large->end(), std::move(func));
}
return ForEach(flat_begin(), flat_end(), std::move(func));
  }

Is a false positive, or is Visual C++ somehow thinking the return value is not 
used when its a lambda? but I get similar issues when compiling with the Intel 
compiler, so I think its more likely that the return value from ForEach isn't 
actually used, and the implementation is just allowing ForEach()'s to be 
chained together

F7673175: image.png 

All of these new warnings introduced are related to the use of a lambda with 
the exception of

  // We don't care what this returns since we'll find out below anyway.
  pool_->TryFindFileInFallbackDatabase(proto.dependency(i));

Which you could say that it should be written as

  [[maybe_unused]] auto rt= 
pool_->TryFindFileInFallbackDatabase(proto.dependency(i));

But the effect of applying the fix-it, doesn't itself break the build, its the 
effect of [[nodiscard]] having been added. (perhaps incorrectly)

However the build is not broken when NOT using warnings as errors, and so it 
depends on the "ethos" of clang-tidy, if the goal is for clang-tidy to remain 
error/warning free after applying -fix then i can see the fix-it generating 
changes that causes warnings isn't correct and that should be considered a 
false positive and I should consider not emitting the [[nodiscard]] when the 
argument is perhaps a lambda.

I guess in the past I've used clang-tidy to also help me generate new warnings 
so I can fix that code, but that may not be the goal of its usage.


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

https://reviews.llvm.org/D55433



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

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D55508#1325316 , @Eugene.Zelenko 
wrote:

> Thank you for this fix!


Your welcome, I was reviewing other revisions to try and get up to speed, and I 
saw you giving someone else the same comment you gave mine!  I'm no python 
expert so this could possible be done a better way, but I hope this at least 
give some relief.


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

https://reviews.llvm.org/D55508



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177496.
MyDeveloperDay added a comment.

Update the documentation to utilize 80 character lines


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

https://reviews.llvm.org/D55433

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseNodiscardCheck.cpp
  clang-tidy/modernize/UseNodiscardCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-nodiscard.rst
  test/clang-tidy/modernize-use-nodiscard.cpp

Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,138 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+
+// TODO using macros to avoid CHECK-FIXES not being able to find [[xxx]]
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
+class Foo
+{
+public:
+bool f1() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f1() const;
+
+bool f2(int) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f2(int) const;
+
+bool f3(const int &) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f3(const int &) const;
+
+bool f4(void) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f4(void) const;
+
+// negative tests
+
+void f5() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: void f5() const;
+
+bool f6();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f6();
+
+bool f7(int &);
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f7(int &);
+
+bool f8(int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f8(int &) const;
+
+bool f9(int *) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f9(int *) const;
+
+bool f10(const int &,int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f10(const int &,int &) const;
+
+NO_DISCARD bool f12() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_DISCARD bool f12() const;
+
+MUST_USE_RESULT bool f13() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: MUST_USE_RESULT bool f13() const;
+   
+// TODO fix CHECK-FIXES to handle "[[" "]]"
+[[nodiscard]] bool f11() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: {{\[\[nodiscard\]\]}} bool f11() const;
+
+bool _f20() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function '_f20' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool _f20() const;
+
+NO_RETURN bool f21() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_RETURN bool f21() const;
+
+~Foo();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: ~Foo();
+
+bool operator +=(int) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool operator +=(int) const;
+   
+// extra keywords (virtual,inline,const) on return type
+
+virtual bool f14() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f14' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD virtual bool f14() const;
+
+const bool f15() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f15' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD const bool f15() const;
+
+inline const bool f16() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f16' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const bool f16() const;
+
+inline virtual const bool f17() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f17' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline virtual const bool f17() const;
+
+// inline with body
+bool f18() const {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f18' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f18() const {
+return true;
+}
+
+bool f19() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f19' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177532.
MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added a comment.

Addressing review comments,

- additional unit tests for no ReplacementString and C++ 11 case
- more expressive hasNonConstReferenceOrPointerArguments matcher
- minor documentation typos and spacing


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

https://reviews.llvm.org/D55433

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseNodiscardCheck.cpp
  clang-tidy/modernize/UseNodiscardCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-nodiscard.rst
  test/clang-tidy/modernize-use-nodiscard-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro.cpp
  test/clang-tidy/modernize-use-nodiscard.cpp

Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,136 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
+class Foo
+{
+public:
+bool f1() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f1() const;
+
+bool f2(int) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f2(int) const;
+
+bool f3(const int &) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f3(const int &) const;
+
+bool f4(void) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f4(void) const;
+
+// negative tests
+
+void f5() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: void f5() const;
+
+bool f6();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f6();
+
+bool f7(int &);
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f7(int &);
+
+bool f8(int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f8(int &) const;
+
+bool f9(int *) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f9(int *) const;
+
+bool f10(const int &,int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f10(const int &,int &) const;
+
+NO_DISCARD bool f12() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_DISCARD bool f12() const;
+
+MUST_USE_RESULT bool f13() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: MUST_USE_RESULT bool f13() const;
+   
+[[nodiscard]] bool f11() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: {{\[\[nodiscard\]\]}} bool f11() const;
+
+bool _f20() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function '_f20' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool _f20() const;
+
+NO_RETURN bool f21() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_RETURN bool f21() const;
+
+~Foo();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: ~Foo();
+
+bool operator +=(int) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool operator +=(int) const;
+   
+// extra keywords (virtual,inline,const) on return type
+
+virtual bool f14() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f14' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD virtual bool f14() const;
+
+const bool f15() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f15' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD const bool f15() const;
+
+inline const bool f16() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f16' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const bool f16() const;
+
+inline virtual const bool f17() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f17' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline virtual const bool f17() const;
+
+// inline with body
+bool f18() const {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f18' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f18() const {
+return tr

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177538.
MyDeveloperDay added a comment.

Fix review comments


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

https://reviews.llvm.org/D55508

Files:
  clang-tidy/add_new_check.py


Index: clang-tidy/add_new_check.py
===
--- clang-tidy/add_new_check.py
+++ clang-tidy/add_new_check.py
@@ -200,23 +200,47 @@
   with open(filename, 'r') as f:
 lines = f.readlines()
 
+  lineMatcher = re.compile('Improvements to clang-tidy')
+  nextSectionMatcher = re.compile('Improvements to include-fixer')
+  checkerMatcher = re.compile('- New :doc:`(.*)')
+
   print('Updating %s...' % filename)
   with open(filename, 'w') as f:
 note_added = False
 header_found = False
+next_header_found = False
+add_note_here = False
 
 for line in lines:
   if not note_added:
-match = re.search('Improvements to clang-tidy', line)
+match = lineMatcher.match(line)
+match_next = nextSectionMatcher.match(line)
+match_checker = checkerMatcher.match(line)
+if match_checker:
+  last_checker = match_checker.group(1)
+  if last_checker > check_name_dashes:
+add_note_here = True
+
+if match_next:
+  next_header_found = True
+  add_note_here = True
+
 if match:
   header_found = True
-elif header_found:
+  f.write(line)
+  continue
+
+if line.startswith(''):
+  f.write(line)
+  continue
+
+if header_found and add_note_here:
   if not line.startswith(''):
-f.write("""
-- New :doc:`%s
+f.write("""- New :doc:`%s
   ` check.
 
   FIXME: add release notes.
+
 """ % (check_name_dashes, check_name_dashes))
 note_added = True
 


Index: clang-tidy/add_new_check.py
===
--- clang-tidy/add_new_check.py
+++ clang-tidy/add_new_check.py
@@ -200,23 +200,47 @@
   with open(filename, 'r') as f:
 lines = f.readlines()
 
+  lineMatcher = re.compile('Improvements to clang-tidy')
+  nextSectionMatcher = re.compile('Improvements to include-fixer')
+  checkerMatcher = re.compile('- New :doc:`(.*)')
+
   print('Updating %s...' % filename)
   with open(filename, 'w') as f:
 note_added = False
 header_found = False
+next_header_found = False
+add_note_here = False
 
 for line in lines:
   if not note_added:
-match = re.search('Improvements to clang-tidy', line)
+match = lineMatcher.match(line)
+match_next = nextSectionMatcher.match(line)
+match_checker = checkerMatcher.match(line)
+if match_checker:
+  last_checker = match_checker.group(1)
+  if last_checker > check_name_dashes:
+add_note_here = True
+
+if match_next:
+  next_header_found = True
+  add_note_here = True
+
 if match:
   header_found = True
-elif header_found:
+  f.write(line)
+  continue
+
+if line.startswith(''):
+  f.write(line)
+  continue
+
+if header_found and add_note_here:
   if not line.startswith(''):
-f.write("""
-- New :doc:`%s
+f.write("""- New :doc:`%s
   ` check.
 
   FIXME: add release notes.
+
 """ % (check_name_dashes, check_name_dashes))
 note_added = True
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

@curdeius Thanks, I don't have commit access so I'm happy wait for a 
CODE_OWNER, they could likely have more input.


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

https://reviews.llvm.org/D55433



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

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 3 inline comments as done.
MyDeveloperDay added a comment.

In D55508#1325670 , @JonasToth wrote:

> I think this patch is fine, AFAIK these utility scripts are not tested 
> directly but are just adjusted if they dont work as expected :)
>
> Did you test it with a fake new-check? If it does what we expect its fine :)


I tested it with both a couple of new fake checks, but also removed everything 
back to how the docs/ReleaseNotes.rst would be after the release branches, i.e. 
with just "The improvements are..."


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

https://reviews.llvm.org/D55508



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

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D55508#1325758 , @JonasToth wrote:

> LGTM. Do you have commit access?


I do not I'm afraid


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

https://reviews.llvm.org/D55508



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177584.
MyDeveloperDay marked 11 inline comments as done.
MyDeveloperDay added a comment.

Addressing review comments

- grammatical errors in documentation and comments
- prevent adding [[nodiscard]] to macros
- clean up list.rst


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

https://reviews.llvm.org/D55433

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseNodiscardCheck.cpp
  clang-tidy/modernize/UseNodiscardCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-nodiscard.rst
  test/clang-tidy/modernize-use-nodiscard-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro.cpp
  test/clang-tidy/modernize-use-nodiscard.cpp

Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,142 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
+#define BOOLEAN_FUNC bool f23() const
+
+class Foo
+{
+public:
+bool f1() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f1() const;
+
+bool f2(int) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f2(int) const;
+
+bool f3(const int &) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f3(const int &) const;
+
+bool f4(void) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f4(void) const;
+
+// negative tests
+
+void f5() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: void f5() const;
+
+bool f6();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f6();
+
+bool f7(int &);
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f7(int &);
+
+bool f8(int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f8(int &) const;
+
+bool f9(int *) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f9(int *) const;
+
+bool f10(const int &,int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f10(const int &,int &) const;
+
+NO_DISCARD bool f12() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_DISCARD bool f12() const;
+
+MUST_USE_RESULT bool f13() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: MUST_USE_RESULT bool f13() const;
+   
+[[nodiscard]] bool f11() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: {{\[\[nodiscard\]\]}} bool f11() const;
+
+bool _f20() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function '_f20' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool _f20() const;
+
+NO_RETURN bool f21() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_RETURN bool f21() const;
+
+~Foo();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: ~Foo();
+
+bool operator +=(int) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool operator +=(int) const;
+   
+// extra keywords (virtual,inline,const) on return type
+
+virtual bool f14() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f14' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD virtual bool f14() const;
+
+const bool f15() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f15' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD const bool f15() const;
+
+inline const bool f16() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f16' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const bool f16() const;
+
+inline virtual const bool f17() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f17' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline virtual const bool f17() const;
+
+// inline with body
+bool f18() const {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f18' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f18() const {
+return true;
+}
+
+   

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177679.
MyDeveloperDay marked 10 inline comments as done.
MyDeveloperDay added a comment.

Resolving some (not all yet) review comments, and looking for help on template 
parameter exclusion

- add additional template argument tests
- add additional clang-warn-unused-result tests
- add additional gcc-warn-unused-result tests
- exclude template parameters
- improve nodiscard wordage in documentation regarding conditions of checker


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

https://reviews.llvm.org/D55433

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseNodiscardCheck.cpp
  clang-tidy/modernize/UseNodiscardCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-nodiscard.rst
  test/clang-tidy/modernize-use-nodiscard-clang-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-gcc-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro.cpp
  test/clang-tidy/modernize-use-nodiscard.cpp

Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,202 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
+#define BOOLEAN_FUNC bool f23() const
+
+typedef unsigned my_unsigned;
+typedef unsigned& my_unsigned_reference;
+typedef const unsigned& my_unsigned_const_reference;
+
+class Foo
+{
+public:
+using size_type = unsigned;
+
+bool f1() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f1() const;
+
+bool f2(int) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f2(int) const;
+
+bool f3(const int &) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f3(const int &) const;
+
+bool f4(void) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f4(void) const;
+
+// negative tests
+
+void f5() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: void f5() const;
+
+bool f6();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f6();
+
+bool f7(int &);
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f7(int &);
+
+bool f8(int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f8(int &) const;
+
+bool f9(int *) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f9(int *) const;
+
+bool f10(const int &,int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f10(const int &,int &) const;
+
+NO_DISCARD bool f12() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_DISCARD bool f12() const;
+
+MUST_USE_RESULT bool f13() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: MUST_USE_RESULT bool f13() const;
+   
+[[nodiscard]] bool f11() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: {{\[\[nodiscard\]\]}} bool f11() const;
+
+bool _f20() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function '_f20' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool _f20() const;
+
+NO_RETURN bool f21() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_RETURN bool f21() const;
+
+~Foo();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: ~Foo();
+
+bool operator +=(int) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool operator +=(int) const;
+   
+// extra keywords (virtual,inline,const) on return type
+
+virtual bool f14() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f14' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD virtual bool f14() const;
+
+const bool f15() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f15' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD const bool f15() const;
+
+inline const bool f16() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f16' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const bool f16() const;
+
+inline v

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I wanted to leave a comment regarding running the [[nodiscard]] checker over 
all of clang... as requested by @JonasToth, @lebedev.ri  and @Szelethus

I'm not 100% sure how to present the results, but let me pick out a few 
high/low lights...

My efforts are somewhat thwarted by the fact I build and develop on windows 
(its ok, I use vim and command line tools!), but I run clang-tidy inside visual 
studio

My first problem is that on windows build of clang LLVM_NODISCARD is #defined 
as nothing unless you tell CMAKE to use C++17  (-DCMAKE_CXX_STANDARD="17" )

Then if you try and make llvm with VS2017 with C++17 turned on, you get into 
all sort of trouble,

You also get into trouble because there are many header files that it add 
LLVM_NODISCARD to but they don't include "Compiler.h" where LLVM is defined (so 
you end up with lots of errors)

3>C:\llvm\include\llvm/ADT/iterator_range.h(46): error C3646: 'IteratorT': 
unknown override specifier (compiling source file 
C:\llvm\tools\clang\utils\TableGen\ClangCommentHTMLNamedCharacterReferenceEmitter.cpp)

I'm wondering if there is anything I can add to the checker, to check to see if 
the macro being used for the ReplacementString is defined "in scope"

Of course as I'm not building with C++17 I could have used [[nodiscard]] 
instead of LLVM_NODISCARD, and that would of improved this I guess

I do get 100's of nodiscard warnings

the majority come from Diag() calls e.g.

  Diag(clang::diag::err_drv_expecting_fopenmp_with_fopenmp_targets);
  
  96>C:\llvm\tools\clang\lib\Driver\Driver.cpp(591): warning C4834: discarding 
return value of function with 'nodiscard' attribute
  96>C:\llvm\tools\clang\lib\Driver\Driver.cpp(688): warning C4834: discarding 
return value of function with 'nodiscard' attribute
  96>C:\llvm\tools\clang\lib\Driver\Driver.cpp(749): warning C4834: discarding 
return value of function with 'nodiscard' attribute
  96>C:\llvm\tools\clang\lib\Driver\Driver.cpp(795): warning C4834: discarding 
return value of function with 'nodiscard' attribute

But I do get some other ones which look interesting, (I don't know these areas 
of the code well enough to know if these are real issues!)

90>C:\llvm\tools\clang\lib\Frontend\DiagnosticRenderer.cpp(476): warning C4834: 
discarding return value of function with 'nodiscard' attribute

  static bool checkRangeForMacroArgExpansion(CharSourceRange Range,
 const SourceManager &SM,
 SourceLocation ArgumentLoc) {
SourceLocation BegLoc = Range.getBegin(), EndLoc = Range.getEnd();
while (BegLoc != EndLoc) {
  if (!checkLocForMacroArgExpansion(BegLoc, SM, ArgumentLoc))
return false;
  BegLoc.getLocWithOffset(1);
}
  
return checkLocForMacroArgExpansion(BegLoc, SM, ArgumentLoc);
  }

also a couple like this

90>C:\llvm\tools\clang\lib\Frontend\SerializedDiagnosticReader.cpp(133): 
warning C4834: discarding return value of function with 'nodiscard' attribute
90>C:\llvm\tools\clang\lib\Frontend\SerializedDiagnosticReader.cpp(175): 
warning C4834: discarding return value of function with 'nodiscard' attribute

  unsigned BlockOrCode = 0;
  llvm::ErrorOr Res = skipUntilRecordOrBlock(Stream, BlockOrCode);
  if (!Res)
Res.getError();

I could see that this level of noise might put people off, although this is 
alot higher level of noise than I saw in both protobuf or in opencv which I 
tried.

I wonder if it would be enough to put in some kind of exclusion regex list?

Reruning the build after having removed the LLVM_NODISCARD from Diag(...) to 
see how much it quietens down

I'll continue looking to see if I find anything interesting.




Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:126
+template
+class Bar
+{

curdeius wrote:
> JonasToth wrote:
> > I think the template tests should be improved.
> > What happens for `T empty()`, `typename T::value_type empty()` and so on. 
> > THe same for the parameters for functions/methods (including function 
> > templates).
> > 
> > Thinking of it, it might be a good idea to ignore functions where the 
> > heuristic depends on the template-paramters.
> It might be a good idea to add a note in the documentation about handling of 
> function templates and functions in class templates.
I think I need some help to determine if the parameter is a template parameter 
(specially a const T & or a const_reference)

I'm trying to remove functions which have any type of Template parameter (at 
least for now)

I've modified the hasNonConstReferenceOrPointerArguments matcher to use 
isTemplateTypeParamType()

but this doesn't seem to work though an Alias or even just with a const &

```
  return llvm::any_of(Node.parameters(), [](const ParmVarDecl *Par) {
QualType ParType = Par->getType();

if (ParType->isTemplateTypeParmType())
  return true;

if (ParType->isPointerType() || isNo

[PATCH] D58404: [clang-format] Add basic support for formatting C# files

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 188239.
MyDeveloperDay added a subscriber: llvm-commits.
MyDeveloperDay added a comment.

Fix a crash running clang-format over large C# code base
Add support for C# Null Coalescing


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

https://reviews.llvm.org/D58404

Files:
  docs/ClangFormat.rst
  docs/ClangFormatStyleOptions.rst
  docs/ReleaseNotes.rst
  include/clang/Basic/LangOptions.def
  include/clang/Basic/TokenKinds.def
  include/clang/Basic/TokenKinds.h
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  lib/Format/FormatTokenLexer.h
  lib/Format/TokenAnnotator.cpp
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineParser.cpp
  lib/Lex/Lexer.cpp
  lib/Lex/LiteralSupport.cpp
  lib/Lex/TokenConcatenation.cpp
  tools/clang-format/ClangFormat.cpp
  unittests/Format/CMakeLists.txt
  unittests/Format/FormatTestCSharp.cpp

Index: unittests/Format/FormatTestCSharp.cpp
===
--- /dev/null
+++ unittests/Format/FormatTestCSharp.cpp
@@ -0,0 +1,176 @@
+//===- unittest/Format/FormatTestCSharp.cpp - Formatting tests for CSharp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "FormatTestUtils.h"
+#include "clang/Format/Format.h"
+#include "llvm/Support/Debug.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+
+class FormatTestCSharp : public ::testing::Test {
+protected:
+  static std::string format(llvm::StringRef Code, unsigned Offset,
+unsigned Length, const FormatStyle &Style) {
+LLVM_DEBUG(llvm::errs() << "---\n");
+LLVM_DEBUG(llvm::errs() << Code << "\n\n");
+std::vector Ranges(1, tooling::Range(Offset, Length));
+tooling::Replacements Replaces = reformat(Style, Code, Ranges);
+auto Result = applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(Result));
+LLVM_DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
+return *Result;
+  }
+
+  static std::string
+  format(llvm::StringRef Code,
+ const FormatStyle &Style = getGoogleStyle(FormatStyle::LK_CSharp)) {
+return format(Code, 0, Code.size(), Style);
+  }
+
+  static FormatStyle getStyleWithColumns(unsigned ColumnLimit) {
+FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
+Style.ColumnLimit = ColumnLimit;
+return Style;
+  }
+
+  static void verifyFormat(
+  llvm::StringRef Code,
+  const FormatStyle &Style = getGoogleStyle(FormatStyle::LK_CSharp)) {
+EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not stable";
+EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
+  }
+};
+
+TEST_F(FormatTestCSharp, CSharpClass) {
+  verifyFormat("public class SomeClass {\n"
+   "  void f() {}\n"
+   "  int g() { return 0; }\n"
+   "  void h() {\n"
+   "while (true) f();\n"
+   "for (;;) f();\n"
+   "if (true) f();\n"
+   "  }\n"
+   "}");
+}
+
+TEST_F(FormatTestCSharp, AccessModifiers) {
+  verifyFormat("public String toString() {}");
+  verifyFormat("private String toString() {}");
+  verifyFormat("protected String toString() {}");
+  verifyFormat("internal String toString() {}");
+
+  verifyFormat("public override String toString() {}");
+  verifyFormat("private override String toString() {}");
+  verifyFormat("protected override String toString() {}");
+  verifyFormat("internal override String toString() {}");
+
+  verifyFormat("internal static String toString() {}");
+}
+
+TEST_F(FormatTestCSharp, NoStringLiteralBreaks) {
+  verifyFormat("foo("
+   "\"a"
+   "aa\");");
+}
+
+TEST_F(FormatTestCSharp, CSharpVerbatiumStringLiterals) {
+  verifyFormat("foo(@\"\\abc\\\");");
+  // @"ABC\" + ToString("B") - handle embedded \ in literal string at
+  // the end
+  verifyFormat("string s = @\"ABC\\\" + ToString(\"B\");");
+}
+
+TEST_F(FormatTestCSharp, CSharpInterpolatedStringLiterals) {
+  verifyFormat("foo($\"{aaa}\");");
+  verifyFormat("foo($\"{A}\");");
+  verifyFormat(
+  "foo($\"{A}"
+  "a\");");
+  verifyFormat("Name = $\"{firstName} {lastName}\";");
+
+  // $"ABC\" + ToString("B") - handle embedded \ in literal string at
+  // the end
+  verifyFormat("string s = $\"A{abc}BC\" + ToString(\"B\");");
+  verifyFormat("$\"{domain}{user}\"");
+  verifyFormat(
+  "var verbatimInterpolated 

[PATCH] D58609: [clang-tidy] bugprone-string-integer-assignment: Reduce false positives.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp:79
+return;
+  }
+

elide {} on small if statements


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58609



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

[PATCH] D56943: [clang-format][NFC] Allow getLLVMStyle() to take a language

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.
Herald added a subscriber: jdoerfert.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:193
   RawStringFormat.Language, &PredefinedStyle)) {
-PredefinedStyle = getLLVMStyle();
+PredefinedStyle = getLLVMStyle(FormatStyle::LK_Cpp);
 PredefinedStyle.Language = RawStringFormat.Language;

if you've set up a default argument of LK_Cpp do you need to keep supplying it? 
if your going to supply it everywhere don't make it a default argument or vice 
versa

This might make a much smaller patch, a little more digestable and all the 
tests can remain as they are.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56943



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

[PATCH] D58731: [clang-tidy] added cppcoreguidelines-explicit-virtual-functions

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Please resubmit this patch with full context

  git diff --cached -U99 > patch_to_submitt.diff

You need to add some documention describing the new option into the check in

docs/clang-tidy/checks/modernize .rst


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58731



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

[PATCH] D56943: [clang-format][NFC] Allow getLLVMStyle() to take a language

[MyDeveloperDay via Phabricator via cfe-commits]

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

I think I've seen you need this for another patch, so in the absence of the 
code owners this LGTM, and thank you for removing all the changes to the tests 
in the previous diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56943



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

[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D28462#1407239 , @micah-s wrote:

> ClamAV recently started using clang-format.  We published this blog post 
> about how we're using it: 
> https://blog.clamav.net/2019/02/clamav-adopts-clang-format.html  One of the 
> things I called out in the blog post is that we really want this feature, and 
> presently we have to use the "// clang-format on/off" markers throughout our 
> code mostly to keep our consecutive macros aligned.
>
> I haven't found time to build clang-format with this patch to test it on our 
> code base with the markers disabled.  If a review of my experience doing so 
> will help get traction for this review in any way, let me know and I'll make 
> the effort.


For those wishing to test the effectiveness of unlanded revisions like this and 
to reduce the amount of time between Windows snapshot builds 
(https://llvm.org/builds/), I have forked llvm-project and using AppVeyor  to 
build a new x64 Windows clang-format-experimental.exe binary, on a 
semi-automatic basis. (master branch clang-format with selected unlanded 
revisions)

https://github.com/mydeveloperday/clang-experimental/releases

When a revision like this meets excessive delay in the review cycle, feel free 
to log a github issue, Early testing of unlanded features may help to prove the 
usefulness of a revision, provide useful review comments and/or help find bugs 
earlier in the development cycle.


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

https://reviews.llvm.org/D28462



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

[PATCH] D58731: [clang-tidy] added cppcoreguidelines-explicit-virtual-functions

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D58731#1412930 , @lewmpk wrote:

> cleaned up documentation


Are you planning on landing this anytime soon given that it was accepted? I 
would like to land D57087: [clang-tidy] add OverrideMacro to 
modernize-use-override check  but I suspect 
either you are I will need to rebase, I'm just trying to be nice and ask if 
you'd like to land first?


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

https://reviews.llvm.org/D58731



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

[PATCH] D58731: [clang-tidy] added cppcoreguidelines-explicit-virtual-functions

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D58731#1413476 , @lewmpk wrote:

> I'm happy to land this ASAP but I don't have commit rights


So one of us could land it for you.. (I've not personally done that before as 
I'm a bit green too! but I do have commit rights)

Its pretty easy to get commit rights, and given your looking at multiple issues 
I'd recommend it..


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

https://reviews.llvm.org/D58731



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

[PATCH] D58731: [clang-tidy] added cppcoreguidelines-explicit-virtual-functions

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D58731#1413695 , @alexfh wrote:

> In D58731#1413552 , @JonasToth wrote:
>
> > In D58731#1413498 , 
> > @MyDeveloperDay wrote:
> >
> > > In D58731#1413476 , @lewmpk 
> > > wrote:
> > >
> > > > I'm happy to land this ASAP but I don't have commit rights
> > >
> > >
> > > So one of us could land it for you.. (I've not personally done that 
> > > before as I'm a bit green too! but I do have commit rights)
> > >
> > > Its pretty easy to get commit rights, and given your looking at multiple 
> > > issues I'd recommend it.. 
> > > (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access)
> >
> >
> > @MyDeveloperDay if you want to try the procedure out, go ahead ;) Otherwise 
> > I can land this patch now.
>
>
> I'll let you folks do this. But please note that the patch wasn't generated 
> with arcanist, so it doesn't apply with `arc patch --nobranch`. Copy-pasting 
> the raw diff to `patch -p0` works though (as would downloading the diff and 
> feeding it to patch as a file). Make sure to put "Differential Revision: 
> https://reviews.llvm.org/D58731"; at the end of the message for Phabricator to 
> close this revision automatically.


@JonasToth  you are more qualified than me please go ahead...


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

https://reviews.llvm.org/D58731



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

[PATCH] D52150: [clang-format] BeforeHash added to IndentPPDirectives

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

While we wait for code review I've landed this currently unaccepted 
clang-format revision into 
https://github.com/mydeveloperday/clang-experimental/releases for those wishing 
to try/test or provide feedback


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

https://reviews.llvm.org/D52150



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

[PATCH] D58731: [clang-tidy] added cppcoreguidelines-explicit-virtual-functions

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

@JonasToth I left a comment in the commit needed to fix the index.rst, which I 
don't think your later review fixes, sphinx complained about the rst file being 
an unreferenced octtree

https://reviews.llvm.org/rG5fbeff797a9dba504f08f14c4fa59b6f1076fe72#inline-2691


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58731



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

[PATCH] D57087: [clang-tidy] add OverrideMacro to modernize-use-override check

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 188752.
MyDeveloperDay added a comment.

rebase after D58731: [clang-tidy] added 
cppcoreguidelines-explicit-virtual-functions  
change

as this was previous accepted I will land shortly, but I'd appreciate a quick 
overview... @JonasToth, @lewmpk


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

https://reviews.llvm.org/D57087

Files:
  clang-tidy/modernize/UseOverrideCheck.cpp
  clang-tidy/modernize/UseOverrideCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/modernize-use-override.rst
  test/clang-tidy/modernize-use-override-with-macro.cpp
  test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp

Index: test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp
@@ -0,0 +1,28 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-override.OverrideSpelling, value: 'CUSTOM_OVERRIDE'},{key: modernize-use-override.FinalSpelling, value: 'CUSTOM_FINAL'}]}" \
+// RUN: -- -std=c++11
+
+// As if the macro was not defined.
+//#define CUSTOM_OVERRIDE override
+//#define CUSTOM_FINAL override
+
+struct Base {
+  virtual ~Base() {}
+  virtual void a();
+  virtual void b();
+};
+
+struct SimpleCases : public Base {
+public:
+  virtual ~SimpleCases();
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using 'CUSTOM_OVERRIDE' or (rarely) 'CUSTOM_FINAL' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  virtual ~SimpleCases();
+
+  void a();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: annotate this function with 'CUSTOM_OVERRIDE' or (rarely) 'CUSTOM_FINAL' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void a();
+
+  virtual void b();
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using 'CUSTOM_OVERRIDE' or (rarely) 'CUSTOM_FINAL' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  virtual void b();
+};
Index: test/clang-tidy/modernize-use-override-with-macro.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-override-with-macro.cpp
@@ -0,0 +1,70 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-override.OverrideSpelling, value: 'OVERRIDE'},{key: modernize-use-override.FinalSpelling, value: 'FINAL'}]}" \
+// RUN: -- -std=c++11
+
+#define ABSTRACT = 0
+
+#define OVERRIDE override
+#define FINAL final
+#define VIRTUAL virtual
+#define NOT_VIRTUAL
+#define NOT_OVERRIDE
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define UNUSED __attribute__((unused))
+
+struct Base {
+  virtual ~Base() {}
+  virtual void a();
+  virtual void b();
+  virtual void c();
+  virtual void e() = 0;
+  virtual void f2() const = 0;
+  virtual void g() = 0;
+  virtual void j() const;
+  virtual void k() = 0;
+  virtual void l() const;
+};
+
+struct SimpleCases : public Base {
+public:
+  virtual ~SimpleCases();
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using 'OVERRIDE' or (rarely) 'FINAL' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  ~SimpleCases() OVERRIDE;
+
+  void a();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: annotate this function with 'OVERRIDE' or (rarely) 'FINAL' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void a() OVERRIDE;
+
+  virtual void b();
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using 'OVERRIDE' or (rarely) 'FINAL' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void b() OVERRIDE;
+
+  virtual void c();
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void c() OVERRIDE;
+
+  virtual void e() = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void e() OVERRIDE = 0;
+
+  virtual void f2() const = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void f2() const OVERRIDE = 0;
+
+  virtual void g() ABSTRACT;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void g() OVERRIDE ABSTRACT;
+
+  virtual void j() const;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void j() const OVERRIDE;
+
+  virtual void k() OVERRIDE;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'OVERRIDE' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void k() OVERRIDE;
+
+  virtual void l() const OVERRIDE;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'OVERRIDE' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void l() const OVERRIDE;
+};
Index: docs/clang-tidy/checks/modernize-use-override.rst
=

[PATCH] D55964: [clang-format][TableGen] Don't add spaces around items in square braces.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D55964



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Hi @reuk,

I'm trying to go over old reviews and see if they are still desired, I stumbled 
on your review and want to see if its still important. Here is some initial 
feedback.

I know its ironic that the clang-format code isn't clang-formatted itself, but 
it might help if your changes didn't also include additional unrelated 
clang-formatting changes because its a little harder to see what actually 
changed, and your review would be much smaller.

perhaps the next steps could be:

1. you rebase to current head (since they've left you hanging her for 3 months!)
2. you use something like git difftool to interactively put back the 
"formatting" of code which isn't part of this change (yes we should really 
clang-format all of clang-format, because I know I've hit this myself recently)
3. The one thing I struggle with when looking at the clang-format code and this 
review (and its probably  part of why the owners seem a bit reluctant to let 
anything else in), is encapsulated by this part of the change



  return Line.Type == LT_ObjCDecl || Left.is(tok::semi) ||
 (Style.SpaceBeforeParens != FormatStyle::SBPO_Never &&
  (Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
tok::kw_switch, tok::kw_case, TT_ForEachMacro,
TT_ObjCForIn) ||
   Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
   (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
 tok::kw_new, tok::kw_delete) &&
(!Left.Previous || Left.Previous->isNot(tok::period) ||
 (Style.SpaceBeforeParens == FormatStyle::SBPO_Always &&
  (Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
   Left.is(tok::r_paren) ||
   (Left.Tok.getIdentifierInfo() &&
Left.Tok.getIdentifierInfo()->isKeyword(
getFormattingLangOpts(Style &&
  Line.Type != LT_PreprocessorDirective) ||
 (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
  Right.ParameterCount >= 1 &&
  (Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
   Left.is(tok::r_square) || Left.is(tok::r_paren) ||
   (Left.Tok.getIdentifierInfo() &&
Left.Tok.getIdentifierInfo()->isKeyword(
getFormattingLangOpts(Style &&
  Line.Type != LT_PreprocessorDirective);

There is nothing wrong with it per say I'm sure it works beautifully but its a 
little hard to reason about.

I know you only adding to the end of it, but I think if this was in clang-tidy 
we'd be asked to write it as a function/matcher to encapsulate this kind of 
functionality. Its also an indictment of clang format that this isn't aligning 
in such a way that it can be more easily read, I mean I know its done its job, 
but its quite a challenge to look at! ;-)

To me clang-format needs more functions like

  Left.isIdentifier()
  Left.isParen()
  Left.isLSquare() 
  Right.hasParameters()

in order to help break complex conditional like this down into a readable form 
which can be rationalised about.

This would help hide some of the tok::XXX complexity which can make my eyes at 
least go funny.. and code like this (which I think you added)

  Left.Tok.getIdentifierInfo() && 
Left.Tok.getIdentifierInfo()->isKeyword(getFormattingLangOpts(Style)))   
(especially as its repeated in the clause)

It looks like it means some means something functional which perhaps could be 
combined:

(sorry I don't know what its doing but I think you do)

  isLanguageKeyWord(Left,Style) 

or

  Left.isLanguageKeyWord(Style);

If you could add the part you added, is a more succinct way,

  || isFunctionWithNoArguments(Left,Right)

rather than

  ||
   (Left.Tok.getIdentifierInfo() &&
Left.Tok.getIdentifierInfo()->isKeyword(
getFormattingLangOpts(Style &&
  Line.Type != LT_PreprocessorDirective) ||
 (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses 
&&
  Right.ParameterCount >= 1 &&
  (Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
   Left.is(tok::r_square) || Left.is(tok::r_paren) ||
   (Left.Tok.getIdentifierInfo() &&
Left.Tok.getIdentifierInfo()->isKeyword(
getFormattingLangOpts(Style &&
  Line.Type != LT_PreprocessorDirective);

I think there would be a stronger argument that it should go in, given that you 
have a publicly available style guide etc... (owners may not agree!)

Perhaps too much of my own opinion here... but maybe the "development costs" 
would go down if thing like

  Left.endsSequence(tok::kw_constexpr, tok::kw_if)

where written as

  Left.isConstExprIf()

if that is what its doing?


Repository:
  rC Clang

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

https://reviews.llvm.or

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

sorry to be critical, just trying to help get traction on your review...




Comment at: lib/Format/TokenAnnotator.cpp:65
 
-const FormatToken &Previous = *CurrentToken->Previous;  // The '<'.
+const FormatToken &Previous = *CurrentToken->Previous; // The '<'.
 if (Previous.Previous) {

unrelated change, consider backing out, perhaps we could do a clang-format of 
the file as a separate pass



Comment at: lib/Format/TokenAnnotator.cpp:526
 if (!Contexts.back().FirstObjCSelectorName) {
-FormatToken* Previous = CurrentToken->getPreviousNonComment();
-if (Previous && Previous->is(TT_SelectorName)) {
-  Previous->ObjCSelectorNameParts = 1;
-  Contexts.back().FirstObjCSelectorName = Previous;
-}
+  FormatToken *Previous = CurrentToken->getPreviousNonComment();
+  if (Previous && Previous->is(TT_SelectorName)) {

unrelated change, consider backing out, perhaps we could do a clang-format of 
the file as a separate pass



Comment at: lib/Format/TokenAnnotator.cpp:1392
+   // FIXME(bug 36976): ObjC return types shouldn't use
+   // TT_CastRParen.
Current.Previous && Current.Previous->is(TT_CastRParen) &&

unrelated change, consider backing out, perhaps we could do a clang-format of 
the file as a separate pass



Comment at: lib/Format/TokenAnnotator.cpp:2420
+Right.MatchingParen->Next &&
+Right.MatchingParen->Next->is(tok::colon);
 return !IsLightweightGeneric && Style.ObjCSpaceBeforeProtocolList;

unrelated change, consider backing out, perhaps we could do a clang-format of 
the file as a separate pass



Comment at: lib/Format/TokenAnnotator.cpp:2545
+  Left.Tok.getIdentifierInfo()->isKeyword(
+  getFormattingLangOpts(Style &&
+Line.Type != LT_PreprocessorDirective) ||

would be nice to pull the 3 items into a function because you repeat it on line 
2551



Comment at: lib/Format/TokenAnnotator.cpp:2548
+   (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
+Right.ParameterCount >= 1 &&
+(Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||

something similar here when combined with line 2540, looks a lot like the 2 
patterns below?

almost like its:


```
bool allowSpaceBeforeIfFunctionHasParameters(Style,Right);
```



Comment at: lib/Format/TokenAnnotator.cpp:2617
+if (Left.MatchingParen &&
+Left.MatchingParen->is(TT_ProtoExtensionLSquare) &&
 Right.isOneOf(tok::l_brace, tok::less))

unrelated change, consider backing out, perhaps we could do a clang-format of 
the file as a separate pass



Comment at: lib/Format/TokenAnnotator.cpp:2833
+   (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
+Right.ParameterCount >= 1);
   if (Right.is(TT_TemplateOpener) && Left.is(tok::r_paren) &&

can this and the above section on line 2745 be combined? I'm not sure if the 
order of the "if" indicates precedence



Comment at: lib/Format/TokenAnnotator.cpp:3138
 if (Right.is(Keywords.kw_is)) {
-  const FormatToken* Next = Right.getNextNonComment();
+  const FormatToken *Next = Right.getNextNonComment();
   // If `is` is followed by a colon, it's likely that it's a dict key, so

unrelated change, consider backing out, perhaps we could do a clang-format of 
the file as a separate pass


Repository:
  rC Clang

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

https://reviews.llvm.org/D55170



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

[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

> ClamAV recently started using clang-format.  We published this blog post 
> about how we're using it: 
> https://blog.clamav.net/2019/02/clamav-adopts-clang-format.html  One of the 
> things I called out in the blog post is that we really want this feature, and 
> presently we have to use the "// clang-format on/off" markers throughout our 
> code mostly to keep our consecutive macros aligned.

Would you please consider logging your struct example from your blog at 
https://bugs.llvm.org,  I see a large number of people using clang format 
on/off in github for this and similar issues (including google code!), looks 
like clang-format needs some struct breaking rules


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

https://reviews.llvm.org/D28462



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

[PATCH] D58819: [clang-format] clang-format off/on not respected when using C Style comments

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: djasper, klimek, JonasToth, krasimir.
MyDeveloperDay added projects: clang, clang-tools-extra.

If the clang-format on/off is in a /* comment */ then the sorting of headers is 
not ignored

PR40901 - https://bugs.llvm.org/show_bug.cgi?id=40901


https://reviews.llvm.org/D58819

Files:
  clang/lib/Format/Format.cpp
  clang/unittests/Format/SortIncludesTest.cpp


Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -117,6 +117,25 @@
  "// clang-format on\n"));
 }
 
+TEST_F(SortIncludesTest, SupportClangFormatOffCStyle) {
+  EXPECT_EQ("#include \n"
+"#include \n"
+"#include \n"
+"/* clang-format off */\n"
+"#include \n"
+"#include \n"
+"#include \n"
+"/* clang-format on */\n",
+sort("#include \n"
+ "#include \n"
+ "#include \n"
+ "/* clang-format off */\n"
+ "#include \n"
+ "#include \n"
+ "#include \n"
+ "/* clang-format on */\n"));
+}
+
 TEST_F(SortIncludesTest, IncludeSortingCanBeDisabled) {
   FmtStyle.SortIncludes = false;
   EXPECT_EQ("#include \"a.h\"\n"
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -1786,9 +1786,11 @@
 Code.substr(Prev, (Pos != StringRef::npos ? Pos : Code.size()) - Prev);
 
 StringRef Trimmed = Line.trim();
-if (Trimmed == "// clang-format off")
+if (Trimmed == "// clang-format off" ||
+Trimmed.startswith("/* clang-format off"))
   FormattingOff = true;
-else if (Trimmed == "// clang-format on")
+else if (Trimmed == "// clang-format on" ||
+ Trimmed.startswith("/* clang-format on"))
   FormattingOff = false;
 
 const bool EmptyLineSkipped =


Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -117,6 +117,25 @@
  "// clang-format on\n"));
 }
 
+TEST_F(SortIncludesTest, SupportClangFormatOffCStyle) {
+  EXPECT_EQ("#include \n"
+"#include \n"
+"#include \n"
+"/* clang-format off */\n"
+"#include \n"
+"#include \n"
+"#include \n"
+"/* clang-format on */\n",
+sort("#include \n"
+ "#include \n"
+ "#include \n"
+ "/* clang-format off */\n"
+ "#include \n"
+ "#include \n"
+ "#include \n"
+ "/* clang-format on */\n"));
+}
+
 TEST_F(SortIncludesTest, IncludeSortingCanBeDisabled) {
   FmtStyle.SortIncludes = false;
   EXPECT_EQ("#include \"a.h\"\n"
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -1786,9 +1786,11 @@
 Code.substr(Prev, (Pos != StringRef::npos ? Pos : Code.size()) - Prev);
 
 StringRef Trimmed = Line.trim();
-if (Trimmed == "// clang-format off")
+if (Trimmed == "// clang-format off" ||
+Trimmed.startswith("/* clang-format off"))
   FormattingOff = true;
-else if (Trimmed == "// clang-format on")
+else if (Trimmed == "// clang-format on" ||
+ Trimmed.startswith("/* clang-format on"))
   FormattingOff = false;
 
 const bool EmptyLineSkipped =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58818: [clang-tidy] added cppcoreguidelines-use-raii-locks check

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp:47
+"cppcoreguidelines-use-raii-locks");
 CheckFactories.registerCheck(
 "cppcoreguidelines-avoid-c-arrays");

Nit: by and large this list looks to be in alphabetical based on the checker 
name (except the last one), not sure if thats by accident or design


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58818



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

[PATCH] D58818: [clang-tidy] added cppcoreguidelines-use-raii-locks check

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

This looks good to me but I would wait for one of @JonasToth or @alexfh to 
perhaps take a look,

maybe you should add some nested scope example too using the same name, I know 
the compiler should warn about a shadow variable anyway but

  std::mutex m;
  m.lock();
  {
  std::mutex m;
  m.lock();
  m.unlock();
  }
  m_unlock();

and what about

  std::mutex m1;
  std::mutex m2;
  
  m1.lock();
  m1.unlock();
  m2.lock();
  m2.unlock();

and

  std::mutex m1;
  std::mutex m2;
  
  m1.lock();
  m2.lock();
  m2.unlock();
  m1.unlock();



  std::mutex m1;
  std::mutex m2;
  
  m1.lock();
  m1.unlock();
  m2.lock();
  m2.unlock();



  std::mutex m1;
  std::mutex m2;
  
  m1.lock();
  m2.lock();
  m1.unlock();
  m2.unlock();


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58818



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

[PATCH] D58819: [clang-format] clang-format off/on not respected when using C Style comments

[MyDeveloperDay via Phabricator via cfe-commits]

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



Comment at: clang/unittests/Format/SortIncludesTest.cpp:132
+ "#include \n"
+ "/* clang-format off */\n"
+ "#include \n"

alexfh wrote:
> Add a test with `/* clang-format officially supports C++ */` ;)
> 
> Seriously speaking, the `startswith()` condition in the code should be a bit 
> stricter. Maybe just compare with `/* clang-format off */` and `/* 
> clang-format on */`? If there's a motivating use case for just checking the 
> prefix, could you add it to the test?
You know i thought I was being so clever by not having the space after ..off to 
avoid

```
/*clang-format off*/
```

That'll teach me!


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

https://reviews.llvm.org/D58819



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

[PATCH] D58819: [clang-format] clang-format off/on not respected when using C Style comments

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 188931.
MyDeveloperDay added a comment.

Fix negative test case
support the same /*clang-format off*/ in the sort includes that 
the TokenLexer supports.


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

https://reviews.llvm.org/D58819

Files:
  clang/lib/Format/Format.cpp
  clang/unittests/Format/SortIncludesTest.cpp


Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -117,6 +117,43 @@
  "// clang-format on\n"));
 }
 
+TEST_F(SortIncludesTest, SupportClangFormatOffCStyle) {
+  EXPECT_EQ("#include \n"
+"#include \n"
+"#include \n"
+"/* clang-format off */\n"
+"#include \n"
+"#include \n"
+"#include \n"
+"/* clang-format on */\n",
+sort("#include \n"
+ "#include \n"
+ "#include \n"
+ "/* clang-format off */\n"
+ "#include \n"
+ "#include \n"
+ "#include \n"
+ "/* clang-format on */\n"));
+
+  // Not really turning it off
+  EXPECT_EQ("#include \n"
+"#include \n"
+"#include \n"
+"/* clang-format offically */\n"
+"#include \n"
+"#include \n"
+"#include \n"
+"/* clang-format onwards */\n",
+sort("#include \n"
+ "#include \n"
+ "#include \n"
+ "/* clang-format offically */\n"
+ "#include \n"
+ "#include \n"
+ "#include \n"
+ "/* clang-format onwards */\n"));
+}
+
 TEST_F(SortIncludesTest, IncludeSortingCanBeDisabled) {
   FmtStyle.SortIncludes = false;
   EXPECT_EQ("#include \"a.h\"\n"
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -1786,9 +1786,10 @@
 Code.substr(Prev, (Pos != StringRef::npos ? Pos : Code.size()) - Prev);
 
 StringRef Trimmed = Line.trim();
-if (Trimmed == "// clang-format off")
+if (Trimmed == "// clang-format off" || Trimmed == "/* clang-format off 
*/")
   FormattingOff = true;
-else if (Trimmed == "// clang-format on")
+else if (Trimmed == "// clang-format on" ||
+ Trimmed == "/* clang-format on */")
   FormattingOff = false;
 
 const bool EmptyLineSkipped =


Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -117,6 +117,43 @@
  "// clang-format on\n"));
 }
 
+TEST_F(SortIncludesTest, SupportClangFormatOffCStyle) {
+  EXPECT_EQ("#include \n"
+"#include \n"
+"#include \n"
+"/* clang-format off */\n"
+"#include \n"
+"#include \n"
+"#include \n"
+"/* clang-format on */\n",
+sort("#include \n"
+ "#include \n"
+ "#include \n"
+ "/* clang-format off */\n"
+ "#include \n"
+ "#include \n"
+ "#include \n"
+ "/* clang-format on */\n"));
+
+  // Not really turning it off
+  EXPECT_EQ("#include \n"
+"#include \n"
+"#include \n"
+"/* clang-format offically */\n"
+"#include \n"
+"#include \n"
+"#include \n"
+"/* clang-format onwards */\n",
+sort("#include \n"
+ "#include \n"
+ "#include \n"
+ "/* clang-format offically */\n"
+ "#include \n"
+ "#include \n"
+ "#include \n"
+ "/* clang-format onwards */\n"));
+}
+
 TEST_F(SortIncludesTest, IncludeSortingCanBeDisabled) {
   FmtStyle.SortIncludes = false;
   EXPECT_EQ("#include \"a.h\"\n"
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -1786,9 +1786,10 @@
 Code.substr(Prev, (Pos != StringRef::npos ? Pos : Code.size()) - Prev);
 
 StringRef Trimmed = Line.trim();
-if (Trimmed == "// clang-format off")
+if (Trimmed == "// clang-format off" || Trimmed == "/* clang-format off */")
   FormattingOff = true;
-else if (Trimmed == "// clang-format on")
+else if (Trimmed == "// clang-format on" ||
+ Trimmed == "/* clang-format on */")
   FormattingOff = false;
 
 const bool EmptyLineSkipped =
___
cfe-commits mailing list
cfe-commits@lists.llvm

[PATCH] D58819: [clang-format] clang-format off/on not respected when using C Style comments

[MyDeveloperDay via Phabricator via cfe-commits]

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



Comment at: clang/lib/Format/Format.cpp:1792
+else if (Trimmed == "// clang-format on" ||
+ Trimmed == "/* clang-format on */")
   FormattingOff = false;

JonasToth wrote:
> Should we allow 
> ```
> /* clang-format off
> It is just horrible for this piece of code. */
> ```
> 
> ? Multiline-comments could span multiple lines and to deactivates 
> clang-format and give reasons.
initially that is kind of what I was trying to do, but if you look over in 
TokenLexer where its turned off for styling they check only for a single line

So to be honest if we are going to change it we should change it in both 
places, but perhaps that is just overkill.


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

https://reviews.llvm.org/D58819



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

[PATCH] D52150: [clang-format] BeforeHash added to IndentPPDirectives

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.

Thank you for helping to address one of the many clang-format issues from 
bugzilla, I'm not the code owner here but this looks good to me. If we want to 
address the many issues we need to be able to move forwards.


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

https://reviews.llvm.org/D52150



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

[PATCH] D57687: [clang-format] Add style option AllowShortLambdasOnASingleLine

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

It might be a good idea to add the nested lambda tests from D44609: 
[Clang-Format] New option BeforeLambdaBody to manage lambda line break inside 
function parameter call (in Allman style)  
into your review to show how this patch copes with those (and the examples from 
the comments of that review)


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

https://reviews.llvm.org/D57687



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

[PATCH] D58922: [clang-format] broken after lambda with return type template with boolean literal

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: klimek, djasper, JonasToth, alexfh, krasimir.
MyDeveloperDay added a project: clang-tools-extra.

A Lamdba with a return type template with a boolean literal (true,false) 
behaves differently to an integer literal

https://bugs.llvm.org/show_bug.cgi?id=40910


https://reviews.llvm.org/D58922

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


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -11846,6 +11846,21 @@
   verifyGoogleFormat("auto a = [&b, c](D* d) -> D& {};");
   verifyGoogleFormat("auto a = [&b, c](D* d) -> const D* {};");
   verifyFormat("[a, a]() -> a<1> {};");
+  verifyFormat("[]() -> a<1> {};");
+  verifyFormat("[]() -> a<1> { ; };");
+  verifyFormat("[]() -> a<1> { ; }();");
+  verifyFormat("[a, a]() -> a {};");
+  verifyFormat("[]() -> a {};");
+  verifyFormat("[]() -> a { ; };");
+  verifyFormat("[]() -> a { ; }();");
+  verifyFormat("[a, a]() -> a {};");
+  verifyFormat("[]() -> a {};");
+  verifyFormat("[]() -> a { ; };");
+  verifyFormat("[]() -> a { ; }();");
+  verifyFormat("auto foo{[]() -> foo { ; }};");
+  verifyFormat("namespace bar {\n"
+   "auto foo{[]() -> foo { ; }};\n"
+   "} // namespace bar");
   verifyFormat("auto  = [](int i, // break for some reason\n"
"   int j) -> int {\n"
"  return (i * 
j);\n"
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1423,6 +1423,8 @@
 case tok::coloncolon:
 case tok::kw_mutable:
 case tok::kw_noexcept:
+case tok::kw_true:
+case tok::kw_false:
   nextToken();
   break;
 case tok::arrow:


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -11846,6 +11846,21 @@
   verifyGoogleFormat("auto a = [&b, c](D* d) -> D& {};");
   verifyGoogleFormat("auto a = [&b, c](D* d) -> const D* {};");
   verifyFormat("[a, a]() -> a<1> {};");
+  verifyFormat("[]() -> a<1> {};");
+  verifyFormat("[]() -> a<1> { ; };");
+  verifyFormat("[]() -> a<1> { ; }();");
+  verifyFormat("[a, a]() -> a {};");
+  verifyFormat("[]() -> a {};");
+  verifyFormat("[]() -> a { ; };");
+  verifyFormat("[]() -> a { ; }();");
+  verifyFormat("[a, a]() -> a {};");
+  verifyFormat("[]() -> a {};");
+  verifyFormat("[]() -> a { ; };");
+  verifyFormat("[]() -> a { ; }();");
+  verifyFormat("auto foo{[]() -> foo { ; }};");
+  verifyFormat("namespace bar {\n"
+   "auto foo{[]() -> foo { ; }};\n"
+   "} // namespace bar");
   verifyFormat("auto  = [](int i, // break for some reason\n"
"   int j) -> int {\n"
"  return (i * j);\n"
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1423,6 +1423,8 @@
 case tok::coloncolon:
 case tok::kw_mutable:
 case tok::kw_noexcept:
+case tok::kw_true:
+case tok::kw_false:
   nextToken();
   break;
 case tok::arrow:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58922: [clang-format] broken after lambda with return type template with boolean literal

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D58922#1417605 , @jkorous wrote:

> BTW I suggest you also add the original test case since the test cases you 
> added fail the expectation at FormatTest.cpp:74 and the message is a bit 
> unclear whether the testcase or the actual implementation is at fault.
>
>   EXPECT_EQ(Expected.str(), format(Expected, Style))
>   << "Expected code is not stable";
>   
>
> The original reported case IMO gives a better idea what is broken since it 
> duplicates the end-of-namespace comment.
>  https://bugs.llvm.org/show_bug.cgi?id=40910


do you mean this case? as this seems to work for me?

  verifyFormat("namespace bar {\n"
 "auto foo{[]() -> foo { ; }};\n"
 "} // namespace bar");



  $ ./tools/clang/unittests/Format/FormatTests.exe 
--gtest_filter=FormatTest.FormatsLambdas*
  Note: Google Test filter = FormatTest.FormatsLambdas*
  [==] Running 1 test from 1 test case.
  [--] Global test environment set-up.
  [--] 1 test from FormatTest
  [ RUN  ] FormatTest.FormatsLambdas
  [   OK ] FormatTest.FormatsLambdas (1353 ms)
  [--] 1 test from FormatTest (1354 ms total)
  
  [--] Global test environment tear-down
  [==] 1 test from 1 test case ran. (1362 ms total)
  [  PASSED  ] 1 test.

BTW if you want me to land this before you land D58934: [clang-format] Fix 
lambdas returning template specialization that contains operator in parameter 
 I need someone to mark this as accepted in 
Phabricator.


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

https://reviews.llvm.org/D58922



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

[PATCH] D58934: [clang-format] Fix lambdas returning template specialization that contains operator in parameter

[MyDeveloperDay via Phabricator via cfe-commits]

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

I'm not sure I personally would ever write code like that ;-) , but if its 
legal C++ and it compiles we should handle it the same as 
foo<1>,foo,foo

As there are a number of reviews out there for formatting Lambdas I think its a 
good idea for us to add corner cases like this to the unit tests, but it does 
get me thinking if this shouldn't be handled by a piece of code which knows 
about trailing return types (template or otherwise) and not be in the general 
Lambda parsing code

I suspect that given that the switch statement handles

  tok::identifier, tok::less, tok::numeric_constant, tok::greater
  foo<  1  >

We are effectively just consuming the return type tokens.

But to handle what you have here it LGTM and handles more use cases that 
https://bugs.llvm.org/show_bug.cgi?id=40910 would throw up.

Thanks for helping out


Repository:
  rC Clang

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

https://reviews.llvm.org/D58934



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

[PATCH] D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: klimek, djasper, JonasToth, alexfh, krasimir.
MyDeveloperDay added a project: clang-tools-extra.
Herald added a subscriber: jdoerfert.

Addressing: PR25010 - https://bugs.llvm.org/show_bug.cgi?id=25010

Code like:

  if(true) var++;
  else  {
  var--;
  }

is reformatted to be

  if (true)
var++;
  else {
var--;
  }

Even when `AllowShortIfStatementsOnASingleLine` is true

The following revision comes from a +1'd suggestion in the PR to support 
AllowShortIfElseStatementsOnASingleLine

This suppresses the clause prevents the merging of the if when there is a 
compound else


https://reviews.llvm.org/D59087

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/lib/Format/Format.cpp
  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
@@ -487,6 +487,34 @@
   verifyFormat("if (a)\n  return;", AllowsMergedIf);
 }
 
+TEST_F(FormatTest, FormatIfWithoutCompoundStatementButElseWith) {
+  FormatStyle AllowsMergedIf = getLLVMStyle();
+  AllowsMergedIf.AlignEscapedNewlines = FormatStyle::ENAS_Left;
+  AllowsMergedIf.AllowShortIfStatementsOnASingleLine = true;
+  verifyFormat("if (a)\n"
+   "  f();\n"
+   "else {\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+
+  AllowsMergedIf.AllowShortIfElseStatementsOnASingleLine = true;
+  verifyFormat("if (a) f();\n"
+   "else {\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+  verifyFormat("if (a) f();\n"
+   "else {\n"
+   "  if (a) f();\n"
+   "  else {\n"
+   "g();\n"
+   "  }\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+}
+
 TEST_F(FormatTest, FormatLoopsWithoutCompoundStatement) {
   FormatStyle AllowsMergedLoops = getLLVMStyle();
   AllowsMergedLoops.AllowShortLoopsOnASingleLine = true;
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -413,10 +413,12 @@
 if (I[1]->First->isOneOf(tok::semi, tok::kw_if, tok::kw_for, tok::kw_while,
  TT_LineComment))
   return 0;
-// Only inline simple if's (no nested if or else).
-if (I + 2 != E && Line.startsWith(tok::kw_if) &&
-I[2]->First->is(tok::kw_else))
-  return 0;
+// Only inline simple if's (no nested if or else), unless specified
+if (!Style.AllowShortIfElseStatementsOnASingleLine) {
+  if (I + 2 != E && Line.startsWith(tok::kw_if) &&
+  I[2]->First->is(tok::kw_else))
+return 0;
+}
 return 1;
   }
 
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -331,6 +331,8 @@
Style.AllowShortFunctionsOnASingleLine);
 IO.mapOptional("AllowShortIfStatementsOnASingleLine",
Style.AllowShortIfStatementsOnASingleLine);
+IO.mapOptional("AllowShortIfElseStatementsOnASingleLine",
+   Style.AllowShortIfElseStatementsOnASingleLine);
 IO.mapOptional("AllowShortLoopsOnASingleLine",
Style.AllowShortLoopsOnASingleLine);
 IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
@@ -632,6 +634,7 @@
   LLVMStyle.AllowShortBlocksOnASingleLine = false;
   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
   LLVMStyle.AllowShortIfStatementsOnASingleLine = false;
+  LLVMStyle.AllowShortIfElseStatementsOnASingleLine = false;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
Index: clang/docs/ClangFormatStyleOptions.rst
===
--- clang/docs/ClangFormatStyleOptions.rst
+++ clang/docs/ClangFormatStyleOptions.rst
@@ -365,11 +365,27 @@
   };
   void f() { bar(); }
 
-
-
 **AllowShortIfStatementsOnASingleLine** (``bool``)
   If ``true``, ``if (a) return;`` can be put on a single line.
 
+  .. code-block:: c++
+
+   if (a) return;
+   else 
+ return;
+
+**AllowShortIfElseStatementsOnASingleLine** (``bool``)
+  When used in conjuction with ``AllowShortIfIfStatementsOnASingleLine``
+  then when ``true``, ``if (a) return;`` can be put on a single even when
+  the else clause is a compound statement
+
+  .. code-block:: c++
+
+   if (a) return;
+   else {
+  return;
+   }
+
 **AllowShortLoopsOnASingleLine** (``bool``)
   If ``true``, ``while (

[PATCH] D59103: [clang-tidy] New checker bugprone-incomplete-comparison-operator

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I definately been burnt by not handling all the cases, but imagine a string 
class

class mystring
{

  const char *ptr;
  int size;
  int numallocated;

}

to compare the strings I wouldn't want to compare all the fields?

I think this could lead to a lot of false positives


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59103



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

[PATCH] D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 189837.
MyDeveloperDay added a comment.

Add missing Format.h from the review


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

https://reviews.llvm.org/D59087

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  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
@@ -487,6 +487,34 @@
   verifyFormat("if (a)\n  return;", AllowsMergedIf);
 }
 
+TEST_F(FormatTest, FormatIfWithoutCompoundStatementButElseWith) {
+  FormatStyle AllowsMergedIf = getLLVMStyle();
+  AllowsMergedIf.AlignEscapedNewlines = FormatStyle::ENAS_Left;
+  AllowsMergedIf.AllowShortIfStatementsOnASingleLine = true;
+  verifyFormat("if (a)\n"
+   "  f();\n"
+   "else {\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+
+  AllowsMergedIf.AllowShortIfElseStatementsOnASingleLine = true;
+  verifyFormat("if (a) f();\n"
+   "else {\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+  verifyFormat("if (a) f();\n"
+   "else {\n"
+   "  if (a) f();\n"
+   "  else {\n"
+   "g();\n"
+   "  }\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+}
+
 TEST_F(FormatTest, FormatLoopsWithoutCompoundStatement) {
   FormatStyle AllowsMergedLoops = getLLVMStyle();
   AllowsMergedLoops.AllowShortLoopsOnASingleLine = true;
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -413,10 +413,12 @@
 if (I[1]->First->isOneOf(tok::semi, tok::kw_if, tok::kw_for, tok::kw_while,
  TT_LineComment))
   return 0;
-// Only inline simple if's (no nested if or else).
-if (I + 2 != E && Line.startsWith(tok::kw_if) &&
-I[2]->First->is(tok::kw_else))
-  return 0;
+// Only inline simple if's (no nested if or else), unless specified
+if (!Style.AllowShortIfElseStatementsOnASingleLine) {
+  if (I + 2 != E && Line.startsWith(tok::kw_if) &&
+  I[2]->First->is(tok::kw_else))
+return 0;
+}
 return 1;
   }
 
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -331,6 +331,8 @@
Style.AllowShortFunctionsOnASingleLine);
 IO.mapOptional("AllowShortIfStatementsOnASingleLine",
Style.AllowShortIfStatementsOnASingleLine);
+IO.mapOptional("AllowShortIfElseStatementsOnASingleLine",
+   Style.AllowShortIfElseStatementsOnASingleLine);
 IO.mapOptional("AllowShortLoopsOnASingleLine",
Style.AllowShortLoopsOnASingleLine);
 IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
@@ -632,6 +634,7 @@
   LLVMStyle.AllowShortBlocksOnASingleLine = false;
   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
   LLVMStyle.AllowShortIfStatementsOnASingleLine = false;
+  LLVMStyle.AllowShortIfElseStatementsOnASingleLine = false;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -244,6 +244,19 @@
   /// If ``true``, ``if (a) return;`` can be put on a single line.
   bool AllowShortIfStatementsOnASingleLine;
 
+  /// When used in conjuction with ``AllowShortIfIfStatementsOnASingleLine``
+  /// then when ``true``, ``if (a) return;`` can be put on a single even when
+  /// the else clause is a compound statement
+  /// \code
+  ///   true:   false:
+  ///   if (a) return;  vs. if (a)
+  ///   else {return;
+  /// return;   else {
+  ///   } return;
+  ///   }
+  /// \endcode
+  bool AllowShortIfElseStatementsOnASingleLine;
+
   /// If ``true``, ``while (true) continue;`` can be put on a single
   /// line.
   bool AllowShortLoopsOnASingleLine;
@@ -1728,6 +1741,8 @@
R.AllowShortFunctionsOnASingleLine &&
AllowShortIfStatementsOnASingleLine ==
R.AllowShortIfStatementsOnASingleLine &&
+   AllowShortIfElseStatementsOnASingleLine ==
+ 

[PATCH] D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 189854.
MyDeveloperDay added a comment.

Fix spelling typo in documentation and comment


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

https://reviews.llvm.org/D59087

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  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
@@ -487,6 +487,34 @@
   verifyFormat("if (a)\n  return;", AllowsMergedIf);
 }
 
+TEST_F(FormatTest, FormatIfWithoutCompoundStatementButElseWith) {
+  FormatStyle AllowsMergedIf = getLLVMStyle();
+  AllowsMergedIf.AlignEscapedNewlines = FormatStyle::ENAS_Left;
+  AllowsMergedIf.AllowShortIfStatementsOnASingleLine = true;
+  verifyFormat("if (a)\n"
+   "  f();\n"
+   "else {\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+
+  AllowsMergedIf.AllowShortIfElseStatementsOnASingleLine = true;
+  verifyFormat("if (a) f();\n"
+   "else {\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+  verifyFormat("if (a) f();\n"
+   "else {\n"
+   "  if (a) f();\n"
+   "  else {\n"
+   "g();\n"
+   "  }\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+}
+
 TEST_F(FormatTest, FormatLoopsWithoutCompoundStatement) {
   FormatStyle AllowsMergedLoops = getLLVMStyle();
   AllowsMergedLoops.AllowShortLoopsOnASingleLine = true;
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -413,10 +413,12 @@
 if (I[1]->First->isOneOf(tok::semi, tok::kw_if, tok::kw_for, tok::kw_while,
  TT_LineComment))
   return 0;
-// Only inline simple if's (no nested if or else).
-if (I + 2 != E && Line.startsWith(tok::kw_if) &&
-I[2]->First->is(tok::kw_else))
-  return 0;
+// Only inline simple if's (no nested if or else), unless specified
+if (!Style.AllowShortIfElseStatementsOnASingleLine) {
+  if (I + 2 != E && Line.startsWith(tok::kw_if) &&
+  I[2]->First->is(tok::kw_else))
+return 0;
+}
 return 1;
   }
 
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -331,6 +331,8 @@
Style.AllowShortFunctionsOnASingleLine);
 IO.mapOptional("AllowShortIfStatementsOnASingleLine",
Style.AllowShortIfStatementsOnASingleLine);
+IO.mapOptional("AllowShortIfElseStatementsOnASingleLine",
+   Style.AllowShortIfElseStatementsOnASingleLine);
 IO.mapOptional("AllowShortLoopsOnASingleLine",
Style.AllowShortLoopsOnASingleLine);
 IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
@@ -632,6 +634,7 @@
   LLVMStyle.AllowShortBlocksOnASingleLine = false;
   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
   LLVMStyle.AllowShortIfStatementsOnASingleLine = false;
+  LLVMStyle.AllowShortIfElseStatementsOnASingleLine = false;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -244,6 +244,19 @@
   /// If ``true``, ``if (a) return;`` can be put on a single line.
   bool AllowShortIfStatementsOnASingleLine;
 
+  /// When used in conjuction with ``AllowShortIfStatementsOnASingleLine``
+  /// then when ``true``, ``if (a) return;`` can be put on a single line even 
+  /// when the else clause is a compound statement.
+  /// \code
+  ///   true:   false:
+  ///   if (a) return;  vs. if (a)
+  ///   else {return;
+  /// return;   else {
+  ///   } return;
+  ///   }
+  /// \endcode
+  bool AllowShortIfElseStatementsOnASingleLine;
+
   /// If ``true``, ``while (true) continue;`` can be put on a single
   /// line.
   bool AllowShortLoopsOnASingleLine;
@@ -1728,6 +1741,8 @@
R.AllowShortFunctionsOnASingleLine &&
AllowShortIfStatementsOnASingleLine ==
R.AllowShortIfStatementsOnASingleLine &&
+   AllowShortIfElseStatementsOnASingl

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[MyDeveloperDay via Phabricator via cfe-commits]

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

I'm not the code owner but this LGTM,

Assuming the unit tests remain passing, I'd like to see more items like this in 
clang-format addressed, at present we seem to lack getting even bug fixes 
reviewed!

Thank you for breaking out the common code, I do think more functions like this 
in clang-format would help make the code more readable. I also think the 
various if clauses having comment helps break up these huge monolithic if 
statements

Thanks for the patch.


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

https://reviews.llvm.org/D55170



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

[PATCH] D33029: [clang-format] add option for dangling parenthesis

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

This review lack unit tests, please add some for FormatTest to show its use 
cases, otherwise someone else will come along and break this later


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

https://reviews.llvm.org/D33029



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D55170#1423798 , @reuk wrote:

> Thanks for the review, and for approving this PR. It's very much appreciated!
>
> I don't have merge rights - could someone commit this for me please?


I'd actively encourage you to go get commit access, I think its much better 
when the commit comes from the author.

There just isn't enough people actively involved in clang-format (and other 
tools) for us to get reviews even looked at (even for defects), we need more 
people involved fixing defects and doing reviews, getting commit access shows 
an intent to fix anything in that comes from your own review which is one of 
the things the code owners push as being an objection to adding more code.


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

https://reviews.llvm.org/D55170



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

[PATCH] D33029: [clang-format] add option for dangling parenthesis

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Adding the unit tests lets us see how this option will work in various cases, 
it will let us understand that its not breaking anything else.

I personally don't like to see revisions like this sit for 2 years with nothing 
happening, I don't see anything wrong with this that would prevent it going in 
so I don't understand whats blocking it?,

if you had some tests and a release note I'd give it a LGTM (but as I've said 
before I'm not the code owner, but someone wanting to address defects and add 
capabilities. but I think we need to be able to move forward, people will 
object soon enough if they don't like it.)

Generally I don't understand why clang-format is so reluctant to change 
anything, As such we don't have many people involved and getting anything done 
(even defects) is extremely hard.

It looks like you met the criteria, and reviewers have been given ample 
opportunity to make an objection. the number of subscribers and like tokens 
would suggest this is wanted,

Please also add a line the in the release notes to say what you are adding. In 
the absence of any other constructive input all we can do is follow the 
guidance on doing a review, for what its worth I notice in the rest of LLVM 
there seems to be a much larger amount of commits that go in even without a 
review, I'm not sure what makes this area so strict, so reluctant to change 
especailly when revisions do seem to be reviewed.


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

https://reviews.llvm.org/D33029



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

[PATCH] D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay planned changes to this revision.
MyDeveloperDay added a comment.

In D59087#1423942 , @reuk wrote:

> Does it make sense to allow `AllowShortIfElseStatementsOnASingleLine` to be 
> enabled when `AllowShortIfStatementsOnASingleLine` is not?
>
> Perhaps it would be better to have `AllowShortIfStatementsOnASingleLine` be 
> represented by an enum with values `Never`, `WithoutElse`, `Always`. The old 
> `true/false` values from config files prior to this change would need to be 
> made to map to the appropriate enum keys.


Thats a good point...its probably more code but I think it might be a better 
approach


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

https://reviews.llvm.org/D59087



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

[PATCH] D33029: [clang-format] add option for dangling parenthesis

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D33029#1423947 , @lebedev.ri wrote:

> In D33029#1423944 , @MyDeveloperDay 
> wrote:
>
> > Adding the unit tests lets us see how this option will work in various 
> > cases, it will let us understand that its not breaking anything else.
> >
> > I personally don't like to see revisions like this sit for 2 years with 
> > nothing happening, I don't see anything wrong with this that would prevent 
> > it going in so I don't understand whats blocking it?,
> >
> > if you had some tests and a release note I'd give it a LGTM (but as I've 
> > said before I'm not the code owner, but someone wanting to address defects 
> > and add capabilities. but I think we need to be able to move forward, 
> > people will object soon enough if they don't like it.)
> >
> > Generally I don't understand why clang-format is so reluctant to change 
> > anything, As such we don't have many people involved and getting anything 
> > done (even defects) is extremely hard.
> >
> > It looks like you met the criteria, and reviewers have been given ample 
> > opportunity to make an objection. the number of subscribers and like tokens 
> > would suggest this is wanted,
> >
> > Please also add a line the in the release notes to say what you are adding. 
> > In the absence of any other constructive input all we can do is follow the 
> > guidance on doing a review, for what its worth I notice in the rest of LLVM 
> > there seems to be a much larger amount of commits that go in even without a 
> > review, I'm not sure what makes this area so strict, so reluctant to change 
> > especailly when revisions do seem to be reviewed.
>
>
> I don't have any stake here, but i just want to point out that no tool 
> (including clang-format)
>  will ever support all the things all the people out there will want it to 
> support. But every
>  new knob is not just a single knob, it needs to work well with all the other 
> existing knobs
>  (and all of the combination of knob params), and every new knob after that.
>
> It's a snowball effect. Things can (and likely will, unless there is at least 
> a *very* strict testing/quality policy
>  (which is 
> https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options
>  about))
>  get out of hand real quickly..
>
> Just 2c.


Correct we should always be add tests and show we don't break existing tests... 
We need to apply the "Beyonce Rule".. "if you liked it you should have put a 
test on it."

We shouldn't just give up making improvements or fixing defects because 
something is hard or complex.


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

https://reviews.llvm.org/D33029



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

[PATCH] D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 190049.
MyDeveloperDay added a comment.

Address review comments

- collapse two options into one


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

https://reviews.llvm.org/D59087

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestSelective.cpp

Index: clang/unittests/Format/FormatTestSelective.cpp
===
--- clang/unittests/Format/FormatTestSelective.cpp
+++ clang/unittests/Format/FormatTestSelective.cpp
@@ -98,7 +98,7 @@
 }
 
 TEST_F(FormatTestSelective, FormatsIfWithoutCompoundStatement) {
-  Style.AllowShortIfStatementsOnASingleLine = true;
+  Style.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse;
   EXPECT_EQ("if (a) return;", format("if(a)\nreturn;", 7, 1));
   EXPECT_EQ("if (a) return; // comment",
 format("if(a)\nreturn; // comment", 20, 1));
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -439,7 +439,7 @@
 
   FormatStyle AllowsMergedIf = getLLVMStyle();
   AllowsMergedIf.AlignEscapedNewlines = FormatStyle::ENAS_Left;
-  AllowsMergedIf.AllowShortIfStatementsOnASingleLine = true;
+  AllowsMergedIf.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse;
   verifyFormat("if (a)\n"
"  // comment\n"
"  f();",
@@ -487,6 +487,34 @@
   verifyFormat("if (a)\n  return;", AllowsMergedIf);
 }
 
+TEST_F(FormatTest, FormatIfWithoutCompoundStatementButElseWith) {
+  FormatStyle AllowsMergedIf = getLLVMStyle();
+  AllowsMergedIf.AlignEscapedNewlines = FormatStyle::ENAS_Left;
+  AllowsMergedIf.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse;
+  verifyFormat("if (a)\n"
+   "  f();\n"
+   "else {\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+
+  AllowsMergedIf.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Always;
+  verifyFormat("if (a) f();\n"
+   "else {\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+  verifyFormat("if (a) f();\n"
+   "else {\n"
+   "  if (a) f();\n"
+   "  else {\n"
+   "g();\n"
+   "  }\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+}
+
 TEST_F(FormatTest, FormatLoopsWithoutCompoundStatement) {
   FormatStyle AllowsMergedLoops = getLLVMStyle();
   AllowsMergedLoops.AllowShortLoopsOnASingleLine = true;
@@ -515,7 +543,7 @@
   AllowSimpleBracedStatements.ColumnLimit = 40;
   AllowSimpleBracedStatements.AllowShortBlocksOnASingleLine = true;
 
-  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = true;
+  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse;
   AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = true;
 
   AllowSimpleBracedStatements.BreakBeforeBraces = FormatStyle::BS_Custom;
@@ -563,7 +591,7 @@
"};",
AllowSimpleBracedStatements);
 
-  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = false;
+  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   verifyFormat("if (true) {}", AllowSimpleBracedStatements);
   verifyFormat("if (true) {\n"
"  f();\n"
@@ -588,7 +616,7 @@
"}",
AllowSimpleBracedStatements);
 
-  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = true;
+  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse;
   AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = true;
   AllowSimpleBracedStatements.BraceWrapping.AfterControlStatement = true;
 
@@ -625,7 +653,7 @@
"}",
AllowSimpleBracedStatements);
 
-  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = false;
+  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   verifyFormat("if (true) {}", AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"
"{\n"
@@ -659,7 +687,7 @@
 TEST_F(FormatTest, ShortBlocksInMacrosDontMergeWithCodeAfterMacro) {
   FormatStyle Style = getLLVMStyleWithColumns(60);
   Style.AllowShortBlocksOnASingleLine = true;
-  Style.AllowShortIfStatementsOnASingleLine = true;
+  Style.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse;
   Style.BreakBeforeBraces = FormatStyle::BS_Allman;
   EXPECT_EQ("#define A  \\\n"
 "  if (HANDLEwernufrnuLwrmviferuvnierv) \\\n"
@@ -3157,7 +3185,7 @@

[PATCH] D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 190053.
MyDeveloperDay marked 8 inline comments as done.
MyDeveloperDay added a comment.

Address review comment

- add document and comment spelling and punctuation
- add additional unit tests to show non compound else clause case


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

https://reviews.llvm.org/D59087

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestSelective.cpp

Index: clang/unittests/Format/FormatTestSelective.cpp
===
--- clang/unittests/Format/FormatTestSelective.cpp
+++ clang/unittests/Format/FormatTestSelective.cpp
@@ -98,7 +98,7 @@
 }
 
 TEST_F(FormatTestSelective, FormatsIfWithoutCompoundStatement) {
-  Style.AllowShortIfStatementsOnASingleLine = true;
+  Style.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse;
   EXPECT_EQ("if (a) return;", format("if(a)\nreturn;", 7, 1));
   EXPECT_EQ("if (a) return; // comment",
 format("if(a)\nreturn; // comment", 20, 1));
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -439,7 +439,7 @@
 
   FormatStyle AllowsMergedIf = getLLVMStyle();
   AllowsMergedIf.AlignEscapedNewlines = FormatStyle::ENAS_Left;
-  AllowsMergedIf.AllowShortIfStatementsOnASingleLine = true;
+  AllowsMergedIf.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse;
   verifyFormat("if (a)\n"
"  // comment\n"
"  f();",
@@ -487,6 +487,41 @@
   verifyFormat("if (a)\n  return;", AllowsMergedIf);
 }
 
+TEST_F(FormatTest, FormatIfWithoutCompoundStatementButElseWith) {
+  FormatStyle AllowsMergedIf = getLLVMStyle();
+  AllowsMergedIf.AlignEscapedNewlines = FormatStyle::ENAS_Left;
+  AllowsMergedIf.AllowShortIfStatementsOnASingleLine =
+  FormatStyle::SIS_WithoutElse;
+  verifyFormat("if (a)\n"
+   "  f();\n"
+   "else {\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+  verifyFormat("if (a)\n"
+   "  f();\n"
+   "else\n"
+   "  g();\n",
+   AllowsMergedIf);
+
+  AllowsMergedIf.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Always;
+
+  verifyFormat("if (a) f();\n"
+   "else {\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+  verifyFormat("if (a) f();\n"
+   "else {\n"
+   "  if (a) f();\n"
+   "  else {\n"
+   "g();\n"
+   "  }\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+}
+
 TEST_F(FormatTest, FormatLoopsWithoutCompoundStatement) {
   FormatStyle AllowsMergedLoops = getLLVMStyle();
   AllowsMergedLoops.AllowShortLoopsOnASingleLine = true;
@@ -515,7 +550,7 @@
   AllowSimpleBracedStatements.ColumnLimit = 40;
   AllowSimpleBracedStatements.AllowShortBlocksOnASingleLine = true;
 
-  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = true;
+  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse;
   AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = true;
 
   AllowSimpleBracedStatements.BreakBeforeBraces = FormatStyle::BS_Custom;
@@ -563,7 +598,7 @@
"};",
AllowSimpleBracedStatements);
 
-  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = false;
+  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   verifyFormat("if (true) {}", AllowSimpleBracedStatements);
   verifyFormat("if (true) {\n"
"  f();\n"
@@ -588,7 +623,7 @@
"}",
AllowSimpleBracedStatements);
 
-  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = true;
+  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse;
   AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = true;
   AllowSimpleBracedStatements.BraceWrapping.AfterControlStatement = true;
 
@@ -625,7 +660,7 @@
"}",
AllowSimpleBracedStatements);
 
-  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = false;
+  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   verifyFormat("if (true) {}", AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"
"{\n"
@@ -659,7 +694,7 @@
 TEST_F(FormatTest, ShortBlocksInMacrosDontMergeWithCodeAfterMacro) {
   FormatStyle Style = getLLVMStyleWithColumns(60);
   Style.AllowShortBlocksOnASingleLine = true;
-  Style.AllowShortIfStatementsOnASingleLine = true;
+  Style.AllowShortIfState

[PATCH] D59210: clang-format: distinguish ObjC call subexpressions after r355434

[MyDeveloperDay via Phabricator via cfe-commits]

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

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D59210



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

[PATCH] D59210: clang-format: distinguish ObjC call subexpressions after r355434

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

most of the cases that we were adding here were for a templated return types

e.g.

  verifyFormat("[]() -> foo<5 + 2> { return {}; };");
  verifyFormat("[]() -> foo<5 - 2> { return {}; };");

So we should probably have handle that "-> (return type)", where the return 
type contains  separately, where those operation tokens are inside the XXX


Repository:
  rC Clang

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

https://reviews.llvm.org/D59210



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

[PATCH] D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 190351.
MyDeveloperDay added a comment.

run git clang-format


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

https://reviews.llvm.org/D59087

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestSelective.cpp

Index: clang/unittests/Format/FormatTestSelective.cpp
===
--- clang/unittests/Format/FormatTestSelective.cpp
+++ clang/unittests/Format/FormatTestSelective.cpp
@@ -98,7 +98,7 @@
 }
 
 TEST_F(FormatTestSelective, FormatsIfWithoutCompoundStatement) {
-  Style.AllowShortIfStatementsOnASingleLine = true;
+  Style.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse;
   EXPECT_EQ("if (a) return;", format("if(a)\nreturn;", 7, 1));
   EXPECT_EQ("if (a) return; // comment",
 format("if(a)\nreturn; // comment", 20, 1));
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -439,7 +439,8 @@
 
   FormatStyle AllowsMergedIf = getLLVMStyle();
   AllowsMergedIf.AlignEscapedNewlines = FormatStyle::ENAS_Left;
-  AllowsMergedIf.AllowShortIfStatementsOnASingleLine = true;
+  AllowsMergedIf.AllowShortIfStatementsOnASingleLine =
+  FormatStyle::SIS_WithoutElse;
   verifyFormat("if (a)\n"
"  // comment\n"
"  f();",
@@ -487,6 +488,41 @@
   verifyFormat("if (a)\n  return;", AllowsMergedIf);
 }
 
+TEST_F(FormatTest, FormatIfWithoutCompoundStatementButElseWith) {
+  FormatStyle AllowsMergedIf = getLLVMStyle();
+  AllowsMergedIf.AlignEscapedNewlines = FormatStyle::ENAS_Left;
+  AllowsMergedIf.AllowShortIfStatementsOnASingleLine =
+  FormatStyle::SIS_WithoutElse;
+  verifyFormat("if (a)\n"
+   "  f();\n"
+   "else {\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+  verifyFormat("if (a)\n"
+   "  f();\n"
+   "else\n"
+   "  g();\n",
+   AllowsMergedIf);
+
+  AllowsMergedIf.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Always;
+
+  verifyFormat("if (a) f();\n"
+   "else {\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+  verifyFormat("if (a) f();\n"
+   "else {\n"
+   "  if (a) f();\n"
+   "  else {\n"
+   "g();\n"
+   "  }\n"
+   "  g();\n"
+   "}",
+   AllowsMergedIf);
+}
+
 TEST_F(FormatTest, FormatLoopsWithoutCompoundStatement) {
   FormatStyle AllowsMergedLoops = getLLVMStyle();
   AllowsMergedLoops.AllowShortLoopsOnASingleLine = true;
@@ -515,7 +551,8 @@
   AllowSimpleBracedStatements.ColumnLimit = 40;
   AllowSimpleBracedStatements.AllowShortBlocksOnASingleLine = true;
 
-  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = true;
+  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine =
+  FormatStyle::SIS_WithoutElse;
   AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = true;
 
   AllowSimpleBracedStatements.BreakBeforeBraces = FormatStyle::BS_Custom;
@@ -563,7 +600,8 @@
"};",
AllowSimpleBracedStatements);
 
-  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = false;
+  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine =
+  FormatStyle::SIS_Never;
   verifyFormat("if (true) {}", AllowSimpleBracedStatements);
   verifyFormat("if (true) {\n"
"  f();\n"
@@ -588,7 +626,8 @@
"}",
AllowSimpleBracedStatements);
 
-  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = true;
+  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine =
+  FormatStyle::SIS_WithoutElse;
   AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = true;
   AllowSimpleBracedStatements.BraceWrapping.AfterControlStatement = true;
 
@@ -625,7 +664,8 @@
"}",
AllowSimpleBracedStatements);
 
-  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = false;
+  AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine =
+  FormatStyle::SIS_Never;
   verifyFormat("if (true) {}", AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"
"{\n"
@@ -659,7 +699,7 @@
 TEST_F(FormatTest, ShortBlocksInMacrosDontMergeWithCodeAfterMacro) {
   FormatStyle Style = getLLVMStyleWithColumns(60);
   Style.AllowShortBlocksOnASingleLine = true;
-  Style.AllowShortIfStatementsOnASingleLine = true;
+  Style.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_WithoutElse;
   Style.BreakBeforeBraces = FormatStyle::BS_Allman;
   EXPECT_EQ("#define A

[PATCH] D59292: [clang-format] messes up indentation when using JavaScript private fields and methods

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: djasper, klimek, JonasToth, reuk, krasimir.
MyDeveloperDay added a project: clang-tools-extra.

Addresses PR40999 https://bugs.llvm.org/show_bug.cgi?id=40999

Private fields and methods  in javasceipt would get incorrectly indented (it 
sees them as preprocessor directives and hence left aligns them)

In this revision  "#identifier" tokens  tok::hash->tok::identifier are merged 
into a single new token
tok::identifier with the '#' contained inside the TokenText

  class Example {
pub = 1;
  #priv = 2;
  
static pub2 = "foo";
static #priv2 = "bar";
  
method() { this.#priv = 5; }
  
static staticMethod() {
  switch (this.#priv) {
  case '1':
  #priv = 3;
break;
  }
}
  
  #privateMethod() {
this.#privateMethod(); // infinite loop
  }
  
  static #staticPrivateMethod() {}
  }

After this fix the code will be correctly indented

  class Example {
pub = 1;
#priv = 2;
  
static pub2 = "foo";
static #priv2 = "bar";
  
method() { this.#priv = 5; }
  
static staticMethod() {
  switch (this.#priv) {
  case '1':
#priv = 3;
break;
  }
}
  
#privateMethod() {
  this.#privateMethod(); // infinite loop
}
  
static #staticPrivateMethod() {}
  }



NOTE: There might be some Javascript code out there which uses the C processor 
to preprocess .js files http://www.nongnu.org/espresso/js-cpp.html,  Its not 
clear how this revision or even private fields and methods would interact.


https://reviews.llvm.org/D59292

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Format/FormatToken.h
  clang/lib/Format/FormatTokenLexer.cpp
  clang/lib/Format/FormatTokenLexer.h
  clang/unittests/Format/FormatTestJS.cpp

Index: clang/unittests/Format/FormatTestJS.cpp
===
--- clang/unittests/Format/FormatTestJS.cpp
+++ clang/unittests/Format/FormatTestJS.cpp
@@ -2328,5 +2328,27 @@
   " never) extends((k: infer I) => void) ? I : never;");
 }
 
-} // end namespace tooling
+TEST_F(FormatTestJS, SupportPrivateFieldsAndMethods) {
+  verifyFormat("class Example {\n"
+   "  pub = 1;\n"
+   "  #priv = 2;\n"
+   "  static pub2 = 'foo';\n"
+   "  static #priv2 = 'bar';\n"
+   "  method() {\n"
+   "this.#priv = 5;\n"
+   "  }\n"
+   "  static staticMethod() {\n"
+   "switch (this.#priv) {\n"
+   "  case '1':\n"
+   "#priv = 3;\n"
+   "break;\n"
+   "}\n"
+   "  }\n"
+   "  #privateMethod() {\n"
+   "this.#privateMethod();  // infinite loop\n"
+   "  }\n"
+   "  static #staticPrivateMethod() {}\n");
+}
+
+} // namespace format
 } // end namespace clang
Index: clang/lib/Format/FormatTokenLexer.h
===
--- clang/lib/Format/FormatTokenLexer.h
+++ clang/lib/Format/FormatTokenLexer.h
@@ -48,6 +48,7 @@
 
   bool tryMergeLessLess();
   bool tryMergeNSStringLiteral();
+  bool tryMergeJSPrivateIdentifier();
 
   bool tryMergeTokens(ArrayRef Kinds, TokenType NewType);
 
Index: clang/lib/Format/FormatTokenLexer.cpp
===
--- clang/lib/Format/FormatTokenLexer.cpp
+++ clang/lib/Format/FormatTokenLexer.cpp
@@ -95,6 +95,8 @@
   Tokens.back()->Tok.setKind(tok::starequal);
   return;
 }
+if (tryMergeJSPrivateIdentifier())
+  return;
   }
 
   if (Style.Language == FormatStyle::LK_Java) {
@@ -121,6 +123,25 @@
   return true;
 }
 
+bool FormatTokenLexer::tryMergeJSPrivateIdentifier() {
+  // Merges #idenfier into a single identifier with the text #identifier
+  // but the token tok::identifier.
+  if (Tokens.size() < 2)
+return false;
+  auto &Hash = *(Tokens.end() - 2);
+  auto &Identifier = *(Tokens.end() - 1);
+  if (!Hash->is(tok::hash) || !Identifier->is(tok::identifier))
+return false;
+  Hash->Tok.setKind(tok::identifier);
+  Hash->TokenText =
+  StringRef(Hash->TokenText.begin(),
+Identifier->TokenText.end() - Hash->TokenText.begin());
+  Hash->ColumnWidth += Identifier->ColumnWidth;
+  Hash->Type = TT_JsPrivateIdentifier;
+  Tokens.erase(Tokens.end() - 1);
+  return true;
+}
+
 bool FormatTokenLexer::tryMergeLessLess() {
   // Merge X,less,less,Y into X,lessless,Y unless X or Y is less.
   if (Tokens.size() < 3)
Index: clang/lib/Format/FormatToken.h
===
--- clang/lib/Format/FormatToken.h
+++ clang/lib/Format/FormatToken.h
@@ -63,6 +63,7 @@
   TYPE(JsTypeColon)\
   TYPE(JsTypeOperator) \
   TYPE(JsTypeO

[PATCH] D59309: [clang-format] BreakAfterReturnType ignored on functions with numeric template parameters

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: djasper, klimek, JonasToth, krasimir, reuk.
MyDeveloperDay added a project: clang-tools-extra.

Addresses PR40696 - https://bugs.llvm.org/show_bug.cgi?id=40696

The BreakAfterReturnType didn't work if it had a single arguments which was a 
template with an integer template parameter

  int  foo(A<8> a) { return a; }

When run with the Mozilla style. would not break after the `int`

  int TestFn(A<8> a)
  {
return a;
  }

This revision resolves this issue by allowing numeric constants to be 
considered function parameters if if seen inside `<>`


https://reviews.llvm.org/D59309

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


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -5419,6 +5419,42 @@
"}\n"
"template  T *f(T &c);\n", // No break here.
Style);
+  verifyFormat("int\n"
+   "foo(A a)\n"
+   "{\n"
+   "  return a;\n"
+   "}\n",
+   Style);
+  verifyFormat("int\n"
+   "foo(A<8> a)\n"
+   "{\n"
+   "  return a;\n"
+   "}\n",
+   Style);
+  verifyFormat("int\n"
+   "foo(A, 8> a)\n"
+   "{\n"
+   "  return a;\n"
+   "}\n",
+   Style);
+  verifyFormat("int\n"
+   "foo(A, bool> a)\n"
+   "{\n"
+   "  return a;\n"
+   "}\n",
+   Style);
+  verifyFormat("int\n"
+   "foo(A, bool> a)\n"
+   "{\n"
+   "  return a;\n"
+   "}\n",
+   Style);
+  verifyFormat("int\n"
+   "foo(A, 8> a)\n"
+   "{\n"
+   "  return a;\n"
+   "}\n",
+   Style);
 }
 
 TEST_F(FormatTest, AlwaysBreakBeforeMultilineStrings) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2040,6 +2040,7 @@
   if (Next->MatchingParen->Next &&
   Next->MatchingParen->Next->is(TT_PointerOrReference))
 return true;
+  int TemplateOpenerLevel = 0;
   for (const FormatToken *Tok = Next->Next; Tok && Tok != Next->MatchingParen;
Tok = Tok->Next) {
 if (Tok->is(tok::l_paren) && Tok->MatchingParen) {
@@ -2049,6 +2050,15 @@
 if (Tok->is(tok::kw_const) || Tok->isSimpleTypeSpecifier() ||
 Tok->isOneOf(TT_PointerOrReference, TT_StartOfName, tok::ellipsis))
   return true;
+// Keep a track of how deep inside nested templates chevrons we are.
+if (Tok->is(TT_TemplateOpener))
+  TemplateOpenerLevel++;
+if (Tok->is(TT_TemplateCloser))
+  TemplateOpenerLevel--;
+// We need to see a numeric_constant in a template argument e.g. <8>
+// for it to be an argument that suggests a function decl.
+if (Tok->is(tok::numeric_constant) && TemplateOpenerLevel > 0)
+  return true;
 if (Tok->isOneOf(tok::l_brace, tok::string_literal, TT_ObjCMethodExpr) ||
 Tok->Tok.isLiteral())
   return false;


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -5419,6 +5419,42 @@
"}\n"
"template  T *f(T &c);\n", // No break here.
Style);
+  verifyFormat("int\n"
+   "foo(A a)\n"
+   "{\n"
+   "  return a;\n"
+   "}\n",
+   Style);
+  verifyFormat("int\n"
+   "foo(A<8> a)\n"
+   "{\n"
+   "  return a;\n"
+   "}\n",
+   Style);
+  verifyFormat("int\n"
+   "foo(A, 8> a)\n"
+   "{\n"
+   "  return a;\n"
+   "}\n",
+   Style);
+  verifyFormat("int\n"
+   "foo(A, bool> a)\n"
+   "{\n"
+   "  return a;\n"
+   "}\n",
+   Style);
+  verifyFormat("int\n"
+   "foo(A, bool> a)\n"
+   "{\n"
+   "  return a;\n"
+   "}\n",
+   Style);
+  verifyFormat("int\n"
+   "foo(A, 8> a)\n"
+   "{\n"
+   "  return a;\n"
+   "}\n",
+   Style);
 }
 
 TEST_F(FormatTest, AlwaysBreakBeforeMultilineStrings) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2040,6 +2040,7 @@
   if (Next->MatchingParen->Next &&
   Next->MatchingParen->Next->is(TT_PointerOrRe

[PATCH] D59332: [clang-format] AlignConsecutiveDeclarations fails with attributes

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: rolandschulz, reuk, djasper, klimek.
MyDeveloperDay added a project: clang-tools-extra.

Addresses http://llvm.org/PR40418

When using `AlignConsecutiveDeclarations: true`

variables with Attributes would not be aligned correctly

  void f(int  extremlylongparametername1,
 long extremlylongparametername2,
 int[[clang::nodiscard]] extremlylongparametername3,
 int __attribute__((clang::nodiscard)) 
extremlylongparametername4) {
int   a;
unsigned long b;
int[[clang::nodiscard]] c;
int __attribute__((clang::nodiscard)) d;
  }

following this change, both parameters and variables with attributes will be 
aligned (space permitting)

  void f(int  extremlylongparametername1,
 long extremlylongparametername2,
 int  [[clang::nodiscard]] 
extremlylongparametername3,
 int __attribute__((clang::nodiscard)) 
extremlylongparametername4) {
int  a;
unsigned longb;
int  [[clang::nodiscard]] c;
int __attribute__((clang::nodiscard)) d;
  }




https://reviews.llvm.org/D59332

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


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -10135,6 +10135,47 @@
"  unsigned c;\n"
"}",
Alignment);
+
+  // See llvm.org/PR40418
+  verifyFormat(
+  "void f(int  extremlylongparametername1,\n"
+  "   long extremlylongparametername2,\n"
+  "   int  [[a::b]] extremlylongparametername3,\n"
+  "   int __attribute__((a::b)) extremlylongparametername4) "
+  "{}\n",
+  Alignment);
+
+  verifyFormat("void f(int  extremlylongparametername1,\n"
+   "   long extremlylongparametername2,\n"
+   "   int  [[deprecated]] "
+   "extremlylongparametername3,\n"
+   "   int __attribute__((deprecated)) "
+   "extremlylongparametername4) "
+   "{}\n",
+   Alignment);
+
+  verifyFormat("int  a;\n"
+   "unsigned longb;\n"
+   "int __attribute__((a::b)) d;\n"
+   "int  [[a::b]] c;\n"
+   "int __attribute__((a::b)) e;\n"
+   "int __attribute__((depreciated)) f;\n"
+   "int __attribute__((aligned(16))) g;\n",
+   Alignment);
+
+  verifyFormat("int  a;\n"
+   "unsigned longb;\n"
+   "int __attribute__((a::b)) d;\n"
+   "int  [[deprecated]] c;\n"
+   "int __attribute__((a::b)) e;\n",
+   Alignment);
+
+  verifyFormat("int  a;\n"
+   "unsigned longb;\n"
+   "int __attribute__((deprecated)) de;\n"
+   "int  [[deprecated]] c;\n"
+   "int __attribute__((a::b)) e;\n",
+   Alignment);
 }
 
 TEST_F(FormatTest, LinuxBraceBreaking) {
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -17,8 +17,8 @@
 namespace clang {
 namespace format {
 
-bool WhitespaceManager::Change::IsBeforeInFile::
-operator()(const Change &C1, const Change &C2) const {
+bool WhitespaceManager::Change::IsBeforeInFile::operator()(
+const Change &C1, const Change &C2) const {
   return SourceMgr.isBeforeInTranslationUnit(
   C1.OriginalWhitespaceRange.getBegin(),
   C2.OriginalWhitespaceRange.getBegin());
@@ -465,6 +465,19 @@
 // definitions.
 return C.Tok->is(TT_StartOfName) ||
C.Tok->is(TT_FunctionDeclarationName) ||
+   C.Tok->startsSequence(
+   tok::l_paren, tok::l_paren, tok::identifier, 
tok::coloncolon,
+   tok::identifier, tok::r_paren, tok::r_paren) ||
+   C.Tok->startsSequence(tok::l_square, tok::l_square,
+ tok::identifier, tok::coloncolon,
+ tok::identifier, tok::r_square,
+ tok::r_square) ||
+   C.Tok->startsSequence(tok::l_square, tok::l_square,
+ tok::identifier, tok::r_square,
+ tok::r_square) ||
+   C.Tok->startsSequence(tok::l_paren, tok::l_paren,
+ tok::identifier, tok::r_paren,
+   

[PATCH] D57435: [clang-tidy] Add abseil-wrap-unique check

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/abseil/WrapUniqueCheck.cpp:7
+// License. See LICENSE.TXT for details.
+//
+//===--===//

License as above



Comment at: clang-tidy/abseil/WrapUniqueCheck.cpp:77
+  return;
+}
+

we don't put braces on small lines, just


```
if (Cons->isListInitialization())
  return;

```



Comment at: clang-tidy/abseil/WrapUniqueCheck.h:6
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//

this is the wrong license wording now (its changed recently..) check out the 
other files in trunk

but its something like this


```
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//

```





Comment at: docs/clang-tidy/checks/abseil-wrap-unique.rst:10
+.. code-block:: c++
+  class A {
+  public:

there is normally a newline after a `code-block`


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57435



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

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:163
+  // Conservatively disable for list initializations
+  if (UseLegacyFunction && New->getInitializationStyle() == 
CXXNewExpr::ListInit) {
+return;

elide the braces



Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:237
+  // Conservatively disable for list initializations
+  if (UseLegacyFunction && New->getInitializationStyle() == 
CXXNewExpr::ListInit) {
+return;

elide the braces



Comment at: docs/clang-tidy/checks/abseil-make-unique.rst:6
+abseil-make-unique
+
+

the  length needs to match the text above it


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

https://reviews.llvm.org/D55044



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