Re: [PATCH] D18852: [clang-tidy] fix a crash with -fdelayed-template-parsing in UnnecessaryValueParamCheck.

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.
LegalizeAdulthood added a comment.

That we've had to fix this twice now tells me that our collective memory is 
forgetful :).

We need to collect the community wisdom for `clang-tidy` gotchas somewhere in 
the docs...


http://reviews.llvm.org/D18852



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

Re: [PATCH] D18783: [clang-tidy] add new checker for string literal with NUL character.

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.
LegalizeAdulthood added a comment.

There are some APIs where a series of strings are passed as a single `char *` 
with NUL separators between the strings and the last string is indicated by two 
NUL characters in a row.  For instance, see the MSDN documentation 

 for the `lpDependencies` argument to `CreateService` 
.
  Such strings literals in the code look like:

  LPCTSTR dependencies = _T("one\0two\0three\0four\0");

I don't see any annotation on this argument in `` where this function 
is declared, so there's no hint from the header about this argument.  In this 
case, the biggest mistake people make is omitting the final NUL character, 
thinking that the `\0` acts as a string separator and not a terminator:

  LPCTSTR dependencies = _T("one\0two\0three\0four");

If you're lucky this results in a crash right away.  If you're unlucky the 
compiler placed this constant in an area of memory with zero padding and it 
works by accident on your machine until it ships to a customer and then breaks 
on their machine mysteriously.

I was never a fan of these magic string array arguments and they mostly appear 
in the older portions of the Win32 API, but they are there nonetheless.  I 
would hate for this check to signal a bunch of false positives on such strings.


http://reviews.llvm.org/D18783



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

Re: [clang-tools-extra] r264563 - clang-tidy: Fix broken buildbot

[Richard via cfe-commits]

In article ,
Alexander Kornienko  writes:
> 
> On Mon, Mar 28, 2016 at 6:15 AM, Richard Thomson via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
> 
> > Author: legalize
> > Date: Sun Mar 27 23:15:41 2016
> > New Revision: 264563
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=264563=rev
> > Log:
> > clang-tidy: Fix broken buildbot
> >
> > VS 2013 does not support char16_t or char32_t

> I'm not sure I understand what this test has to do with VS2013.

I incorrectly stated it as VS 2013 and not the Windows buildbot
configuration as being the root cause.

If you look at the failed build message, it is clearly saying "error:
unknown type name 'char16_t'" and "error: unknown type name 'char32_t'".


I could either do something that would fix the build quickly (which is
what I chose to do), or I could reverse out the entire changeset.

I chose the former as the best means of getting the builds green
again.  The existing raw string literal check doesn't yet process
character literals other than ASCII literals, so there is nothing lost
in commenting out these lines of the test file.

> Clang-tidy
> should be able to parse this code, and if it doesn't (e.g. due to
> -fms-compatibility being turned on by default on windows), we should put
> unsupported constructs under an appropriate #ifdef, not comment them out
> completely.

That is why I added a TODO comment instead of just removing the lines.

Feel free to patch it such that it does some kind of CMake based
feature check for char16_t and char32_t and then configures a header
file with HAS_xxx style check macros, include that generated header
file in the test sources and then use the preprocessor to conditionally
include the lines in the test file based on compiler support.

To me, that is a significant chunk of work, which is why I opted
instead for commenting out these (not too important yet) lines of test
code and add a TODO comment.  That TODO can be addressed when the
check is expanded to handle non-ASCII string literals.
-- 
"The Direct3D Graphics Pipeline" free book 
 The Computer Graphics Museum 
 The Terminals Wiki 
  Legalize Adulthood! (my blog) 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D17482#390237, @alexfh wrote:

> My main concern is that this change makes the already complicated and 
> unobvious testing mechanism [...]


The complexity and obtuseness of the existing testing mechanism is unrelated to 
this changeset.  This changeset doesn't fundamentally change the testing 
mechanism.

> even more complicated and even less obvious for a very little (as I see it 
> now) benefit.


The benefit is that you would now have a consistent mechanism for verifying 
changes made to headers.

> The added functionality supports a very limited use case (a single header)


You have to start somewhere.  Currently there is no mechanism provided at all.  
Saying that this mechanism is not acceptable because it doesn't handle 
arbitrarily complex generalized checking is making the perfect the enemy of the 
good and isn't a reason for preventing this change from being accepted IMO.

> and it does it in a rather hacky way (the use of the new --header-filter flag 
> is not self-documenting and the overall behavior seems "magical" at the first 
> glance).


None of the mechanisms in the testing of these files is self-documenting.  If 
that is the criteria by which all changes are to be measured, then the entire 
existing system has to be thrown out.  Again, this feels like making the 
perfect the enemy of the good.  The behavior for validating header files is the 
same as the existing behavior for validating source files.  They are copied, 
filtered, processed and the processed results are analyzed.  Discrepencies are 
shown as diffs against the original source files.

> There's also an outstanding comment that you didn't seem to have addressed 
> yet.


Which specific comment are you referring to?  Because, just like this comment, 
there's a bunch of discussion in very general terms without specific complaints 
against specific pieces of code that are preventing it from being committed.

> In http://reviews.llvm.org/D17482#378685, @LegalizeAdulthood wrote:

> 

> > In the context of a clang-tidy check, what would be gained by verifying 
> > changes made to multiple headers that isn't already tested by verifying 
> > changes made to a single header?

> 


You haven't answered this question yet.  The existing test mechanism verifies 
only a single source file at a time; I see no reason why verifying a single 
header is such a great imposition of constraints that would prevent existing 
checks from being enhanced to verify their changes on header files.  However, 
should that arise in the future it is a relatively small change to add 
additional header processing to the script.  Again, let's not make the perfect 
the enemy of the good.  There is no reason we cannot improve the code in steps, 
on demand, as the need arises.  This is the essence of agile development.  
YAGNI 

> > At least two of the checks I've already added have complaints that they 
> > don't work on headers.

> 

> 

> That's because you used `isExpansionInMainFile`, which is just not needed 
> there


...and that was because I couldn't write an automated test against header files 
to verify the changes made there.  The whole point of THIS changeset was to 
give me a mechanism for verifying the fixes in a header so that I could address 
those issues in the bug database.

But instead of getting on with fixing it, I've spent 6 weeks waiting for this 
changeset to be reviewed and that discussion has prevented an advancement of 
functionality in the testing framework.

> > While this change was waiting to be reviewed, another check was added that 
> > made changes to headers but had no way to verify them.

> 

> 

> Which check do you mean? Does it need a special treatment of headers or is it 
> just able to make fixes in headers as most other checks?


I already quoted the check in question earlier in this thread; please review 
those messages.


http://reviews.llvm.org/D17482



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

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

As it stands currently, you can't commit a header with `CHECK-MESSAGES` and 
`CHECK-FIXES` lines and have them verified.

That's the whole point of this changeset.

Currently you have to do something very hacky in order to verify changes made 
to headers.


http://reviews.llvm.org/D17482



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

Re: [PATCH] D18509: clang-tidy: add_new_check.py stubs out release notes

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D18509#388071, @hintonda wrote:

> With this change, won't you need to update 2 different repos: clang and 
> extra?  Is it possible to update both with a single Phabricator patch?


This is updating the release notes in the extra repository.  (The release notes 
are new to this repository; you may not have noticed them being added.)


http://reviews.llvm.org/D18509



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

Re: [PATCH] D18608: note for top-level consts in function decls tidy

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: docs/ReleaseNotes.rst:153-154
@@ -152,2 +152,4 @@
 
+  * `readability-avoid-const-params-in-decls
+
`_
   * `readability-identifier-naming

fowles wrote:
> LegalizeAdulthood wrote:
> > Was this added in 3.8 or is it being added in 3.9?
> > 
> > That URL gives me 404 not found.
> > 
> > If it is new to 3.9, it should be farther up in the file.
> I don't know.  It was committed yesterday.
Hrm.  I see the page describing it here:
http://clang.llvm.org/extra/clang-tidy/checks/readability-avoid-const-params-in-decls.html
but I think those docs are continuously rebuilt from trunk.

The release specific URL you added does not work and if you look in the base 
directory for that URL:
http://llvm.org/releases/3.8.0/tools/clang/tools/extra/docs/clang-tidy/checks/
you will not find a file for readability-avoid-const-params-in-decls

I suspect it means that the documentation was added since the 3.8 release and 
therefore the check isn't actually available in the 3.8 release either.  Yep, 
looking at the github mirror for branch release_38, this check isn't present.  
See the [[ 
https://github.com/llvm-mirror/clang-tools-extra/tree/release_38/clang-tidy/readability
 | mirror on github ]] to verify this.

Therefore, I conclude that the check should be listed in the what's new for 3.9 
section and not what was added between 3.7 and 3.8.


http://reviews.llvm.org/D18608



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

Re: [PATCH] D18582: [Clang-tidy] Update release notes with list of checks added since 3.8

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: docs/ReleaseNotes.rst:143
@@ -71,3 +142,3 @@
 Improvements to ``modularize``
 ^^
 

LegalizeAdulthood wrote:
> The formatting here has been changed on trunk.  Please rebase with a full 
> file context so we can see the whole file in phabricator.
Alex changed:

```
Improvements to ``modularize``
^^
```
to
```
Improvements to modularize
--
```
(See [[ 
https://raw.githubusercontent.com/llvm-mirror/clang-tools-extra/master/docs/ReleaseNotes.rst
 | raw version on github mirror ]])

and your diff is against the old version, so you will want to rebase the 
changeset and post a full context diff when you update this review.


Repository:
  rL LLVM

http://reviews.llvm.org/D18582



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

Re: [PATCH] D18608: note for top-level consts in function decls tidy

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.


Comment at: docs/ReleaseNotes.rst:153-154
@@ -152,2 +152,4 @@
 
+  * `readability-avoid-const-params-in-decls
+
`_
   * `readability-identifier-naming

Was this added in 3.8 or is it being added in 3.9?

That URL gives me 404 not found.

If it is new to 3.9, it should be farther up in the file.


http://reviews.llvm.org/D18608



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

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

I'm sorry to be such a nag and I know you're busy, but

This changeset has been held up for a month and a half. (6 weeks since 
originally posted in http://reviews.llvm.org/D16953)

The change isn't that complicated and there haven't really been any comments 
requiring action on my part, so I don't understand why this is taking so long.


http://reviews.llvm.org/D17482



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

Re: [PATCH] D18509: clang-tidy: add_new_check.py stubs out release notes

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: clang-tidy/add_new_check.py:283
@@ +282,3 @@
+  elif not clang_tidy_heading_found:
+if line.startswith('^^'):
+  clang_tidy_heading_found = True

Alex changed the formatting, so this will need to be rebased.


http://reviews.llvm.org/D18509



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

Re: [PATCH] D18584: Complete support for C++ Core Guidelines Type.6: Always initialize a member variable.

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.
LegalizeAdulthood added a comment.

Can you summarize the improvements in `docs/ReleaseNotes.rst`, please?


http://reviews.llvm.org/D18584



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

Re: [PATCH] D18582: [Clang-tidy] Update release notes with list of checks added since 3.8

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: docs/ReleaseNotes.rst:143
@@ -71,3 +142,3 @@
 Improvements to ``modularize``
 ^^
 

The formatting here has been changed on trunk.  Please rebase with a full file 
context so we can see the whole file in phabricator.


Repository:
  rL LLVM

http://reviews.llvm.org/D18582



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

Re: [PATCH] D18475: [clang-tidy] Add more detection rules for redundant c_str calls.

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.
LegalizeAdulthood added a comment.

Can you summarize the improvements in `docs/ReleaseNotes.rst` please?


http://reviews.llvm.org/D18475



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

Re: [PATCH] D18457: [clang-tidy] Add a new checker to detect missing comma in initializer list.

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.
LegalizeAdulthood added a comment.

Can you summarize this check in `docs/ReleaseNotes.rst` in the clang-tidy 
section, please?


http://reviews.llvm.org/D18457



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

Re: [PATCH] D18408: readability check for const params in declarations

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Maybe you need to rebase first?  I haven't used arc.


Repository:
  rL LLVM

http://reviews.llvm.org/D18408



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

Re: [PATCH] D18136: boost-use-to-string check

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Please summarize this check in `docs/ReleaseNotes.rst` in the clang-tidy 
section.


http://reviews.llvm.org/D18136



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

Re: [PATCH] D18396: Clang-tidy:modernize-use-override. Fix for __declspec attributes and const=0 without spacse

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.
LegalizeAdulthood added a comment.

Please summarize the improvements in `docs/ReleaseNotes.rst` in the clang tidy 
section.


http://reviews.llvm.org/D18396



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

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.
LegalizeAdulthood added a comment.

Please summarize this check in `docs/ReleaseNotes.rst`.

There is a pending review that will stub this out automatically, but it hasn't 
been approved yet


http://reviews.llvm.org/D18265



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

Re: [PATCH] D18408: readability check for const params in declarations

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.
LegalizeAdulthood added a comment.

You can submit the release notes changes in a new patch.


Repository:
  rL LLVM

http://reviews.llvm.org/D18408



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

Re: [PATCH] D17811: [clang-tidy] Add check to detect dangling references in value handlers.

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.
LegalizeAdulthood added a comment.

Can you add a synopsis of this new check to `docs/ReleaseNotes.rst` please?


Repository:
  rL LLVM

http://reviews.llvm.org/D17811



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

Re: [PATCH] D7982: Add readability-duplicate-include check to clang-tidy

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: clang-tidy/readability/DuplicateIncludeCheck.cpp:62
@@ +61,3 @@
+StringRef SearchPath, StringRef RelativePath, const Module *Imported) {
+  if (!SM_.isInMainFile(HashLoc)) {
+return;

LegalizeAdulthood wrote:
> LegalizeAdulthood wrote:
> > alexfh wrote:
> > > LegalizeAdulthood wrote:
> > > > alexfh wrote:
> > > > > What's the reason to limit the check to the main file only? I think, 
> > > > > it should work on all headers as well. Also, sometimes it's fine to 
> > > > > have duplicate includes even without macro definitions in between, 
> > > > > e.g. when these #includes are in different namespaces.
> > > > > 
> > > > > I'd suggest using the same technique as in the IncludeOrderCheck: for 
> > > > > each file collect all preprocessor directives sorted by 
> > > > > SourceLocation. Then detect #include blocks (not necessarily the same 
> > > > > way as its done in the IncludeOrderCheck. Maybe use the presense of 
> > > > > any non-comment tokens between #includes as a boundary of blocks), 
> > > > > and detect duplicate includes in each block.
> > > > If I remove the `isInMainFile`, how do I ensure that I don't attempt to 
> > > > modify system headers?
> > > Using `SourceLocation::isInSystemHeader()`.
> > Thanks, I'll try that.  The next question that brings to mind is how do I 
> > distinguish between headers that I own and headers used from third party 
> > libraries?
> > 
> > For instance, suppose I run a check on a clang refactoring tool and it uses 
> > `isInSystemHeader` and starts flagging issues in the clang tooling library 
> > headers.
> > 
> > The `compile_commands.json` doesn't contain any information about headers 
> > in my project, only translation units in my build, so it doesn't know 
> > whether or not included headers belong to me or third-party libraries.
> For the benefit of others reading this, Alex pointed out to me the 
> `-header-filter` and `-system-headers` options to clang-tidy.  I think this 
> means I don't need any narrowing if `isExpansinInMainFile()` in any of my 
> matchers.  I will do some experimenting to verify that this doesn't introduce 
> regressions.
If I remove the `isInMainFile` and replace it with a check to 
`isInSystemHeader`, then all my tests fail.  I will have to investigate this 
further.


http://reviews.llvm.org/D7982



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

Re: [PATCH] D17491: Add performance check to flag function parameters of expensive to copy types that can be safely converted to const references.

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.
LegalizeAdulthood added a comment.

Can you add something to the docs/ReleaseNotes.rst for this new check, please?


Repository:
  rL LLVM

http://reviews.llvm.org/D17491



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

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Squeak


http://reviews.llvm.org/D17482



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

[PATCH] D18509: clang-tidy: add_new_check.py stubs out release notes

[Richard via cfe-commits]

LegalizeAdulthood created this revision.
LegalizeAdulthood added a reviewer: alexfh.
LegalizeAdulthood added a subscriber: cfe-commits.

Update `add_new_check.py` to stub out information in the release notes for the 
new check.

http://reviews.llvm.org/D18509

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
@@ -261,6 +261,38 @@
 """ % {"check_name_dashes" : check_name_dashes,
"underline" : "=" * len(check_name_dashes)})
 
+
+# Adds the new check to the release notes for clang-tidy
+def adapt_release_notes(module_path, module, check_name):
+  check_name_dashes = module + '-' + check_name
+  filename = os.path.normpath(
+os.path.join(module_path, '../../docs/ReleaseNotes.rst'))
+  print('Updating %s...' % filename)
+  with open(filename, 'r') as release_notes:
+lines = release_notes.readlines()
+  with open(filename, 'w') as release_notes:
+clang_tidy_found = False
+clang_tidy_heading_found = False
+bullet_found = False
+
+for line in lines:
+  if not clang_tidy_found:
+if line.startswith("Improvements to ``clang-tidy``"):
+  clang_tidy_found = True
+  elif not clang_tidy_heading_found:
+if line.startswith('^^'):
+  clang_tidy_heading_found = True
+  elif not bullet_found:
+if line.startswith('- '):
+  bullet_found = True
+  release_notes.write('- New ``' + check_name_dashes + '`` check\n')
+  release_notes.write('\n')
+  release_notes.write('  FIXME: summarize the check\n')
+  release_notes.write('\n')
+  release_notes.write(line)
+  pass
+
+
 def main():
   if len(sys.argv) == 2 and sys.argv[1] == '--update-docs':
 update_checks_list(os.path.dirname(sys.argv[0]))
@@ -289,6 +321,7 @@
   adapt_module(module_path, module, check_name, check_name_camel)
   write_test(module_path, module, check_name)
   write_docs(module_path, module, check_name)
+  adapt_release_notes(module_path, module, check_name)
   update_checks_list(clang_tidy_path)
   print('Done. Now it\'s your turn!')
 


Index: clang-tidy/add_new_check.py
===
--- clang-tidy/add_new_check.py
+++ clang-tidy/add_new_check.py
@@ -261,6 +261,38 @@
 """ % {"check_name_dashes" : check_name_dashes,
"underline" : "=" * len(check_name_dashes)})
 
+
+# Adds the new check to the release notes for clang-tidy
+def adapt_release_notes(module_path, module, check_name):
+  check_name_dashes = module + '-' + check_name
+  filename = os.path.normpath(
+os.path.join(module_path, '../../docs/ReleaseNotes.rst'))
+  print('Updating %s...' % filename)
+  with open(filename, 'r') as release_notes:
+lines = release_notes.readlines()
+  with open(filename, 'w') as release_notes:
+clang_tidy_found = False
+clang_tidy_heading_found = False
+bullet_found = False
+
+for line in lines:
+  if not clang_tidy_found:
+if line.startswith("Improvements to ``clang-tidy``"):
+  clang_tidy_found = True
+  elif not clang_tidy_heading_found:
+if line.startswith('^^'):
+  clang_tidy_heading_found = True
+  elif not bullet_found:
+if line.startswith('- '):
+  bullet_found = True
+  release_notes.write('- New ``' + check_name_dashes + '`` check\n')
+  release_notes.write('\n')
+  release_notes.write('  FIXME: summarize the check\n')
+  release_notes.write('\n')
+  release_notes.write(line)
+  pass
+
+
 def main():
   if len(sys.argv) == 2 and sys.argv[1] == '--update-docs':
 update_checks_list(os.path.dirname(sys.argv[0]))
@@ -289,6 +321,7 @@
   adapt_module(module_path, module, check_name, check_name_camel)
   write_test(module_path, module, check_name)
   write_docs(module_path, module, check_name)
+  adapt_release_notes(module_path, module, check_name)
   update_checks_list(clang_tidy_path)
   print('Done. Now it\'s your turn!')
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 51738.
LegalizeAdulthood added a comment.

Remove brace initializers from test code.


http://reviews.llvm.org/D16529

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/RawStringLiteralCheck.cpp
  clang-tidy/modernize/RawStringLiteralCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-raw-string-literal.rst
  test/clang-tidy/modernize-raw-string-literal-delimiter.cpp
  test/clang-tidy/modernize-raw-string-literal.cpp

Index: test/clang-tidy/modernize-raw-string-literal.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-raw-string-literal.cpp
@@ -0,0 +1,123 @@
+// RUN: %check_clang_tidy %s modernize-raw-string-literal %t
+
+char const *const BackSlash("goink\\frob");
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: escaped string literal can be written as a raw string literal [modernize-raw-string-literal]
+// CHECK-FIXES: {{^}}char const *const BackSlash(R"(goink\frob)");{{$}}
+
+char const *const PlainLiteral("plain literal");
+
+// Non-printable ASCII characters.
+char const *const Nul("goink\\\000");
+char const *const Soh("goink\\\001");
+char const *const Stx("goink\\\002");
+char const *const Etx("goink\\\003");
+char const *const Enq("goink\\\004");
+char const *const Ack("goink\\\005");
+char const *const Bell("goink\\\afrob");
+char const *const BackSpace("goink\\\bfrob");
+char const *const HorizontalTab("goink\\\tfrob");
+char const *const NewLine("goink\nfrob");
+char const *const VerticalTab("goink\\\vfrob");
+char const *const FormFeed("goink\\\ffrob");
+char const *const CarraigeReturn("goink\\\rfrob");
+char const *const So("goink\\\016");
+char const *const Si("goink\\\017");
+char const *const Dle("goink\\\020");
+char const *const Dc1("goink\\\021");
+char const *const Dc2("goink\\\022");
+char const *const Dc3("goink\\\023");
+char const *const Dc4("goink\\\024");
+char const *const Nak("goink\\\025");
+char const *const Syn("goink\\\026");
+char const *const Etb("goink\\\027");
+char const *const Can("goink\\\030");
+char const *const Em("goink\\\031");
+char const *const Sub("goink\\\032");
+char const *const Esc("goink\\\033");
+char const *const Fs("goink\\\034");
+char const *const Gs("goink\\\035");
+char const *const Rs("goink\\\036");
+char const *const Us("goink\\\037");
+char const *const HexNonPrintable("\\\x03");
+char const *const Delete("\\\177");
+
+char const *const TrailingSpace("A line \\with space. \n");
+char const *const TrailingNewLine("A single \\line.\n");
+char const *const AlreadyRaw(R"(foobie\\bletch)");
+char const *const UTF8Literal(u8"foobie\\bletch");
+char const *const UTF8RawLiteral(u8R"(foobie\\bletch)");
+char16_t const *const UTF16Literal(u"foobie\\bletch");
+char16_t const *const UTF16RawLiteral(uR"(foobie\\bletch)");
+char32_t const *const UTF32Literal(U"foobie\\bletch");
+char32_t const *const UTF32RawLiteral(UR"(foobie\\bletch)");
+wchar_t const *const WideLiteral(L"foobie\\bletch");
+wchar_t const *const WideRawLiteral(LR"(foobie\\bletch)");
+
+char const *const SingleQuote("goink\'frob");
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
+// CHECK-XFIXES: {{^}}char const *const SingleQuote(R"(goink'frob)");{{$}}
+
+char const *const DoubleQuote("goink\"frob");
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const DoubleQuote(R"(goink"frob)");{{$}}
+
+char const *const QuestionMark("goink\?frob");
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const QuestionMark(R"(goink?frob)");{{$}}
+
+char const *const RegEx("goink\\(one|two\\)\\?.*\\nfrob");
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const RegEx(R"(goink\(one|two\)\\\?.*\nfrob)");{{$}}
+
+char const *const Path("C:\\Program Files\\Vendor\\Application\\Application.exe");
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const Path(R"(C:\Program Files\Vendor\Application\Application.exe)");{{$}}
+
+char const *const ContainsSentinel("who\\ops)\"");
+// CHECK-MESSAGES: :[[@LINE-1]]:36: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const ContainsSentinel(R"lit(who\ops)")lit");{{$}}
+
+char const *const ContainsDelim("whoops)\")lit\"");
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const ContainsDelim(R"lit1(whoops)")lit")lit1");{{$}}
+
+char const *const OctalPrintable("\100\\");
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char 

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 51727.
LegalizeAdulthood added a comment.

Update release notes


http://reviews.llvm.org/D16529

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/RawStringLiteralCheck.cpp
  clang-tidy/modernize/RawStringLiteralCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-raw-string-literal.rst
  test/clang-tidy/modernize-raw-string-literal-delimiter.cpp
  test/clang-tidy/modernize-raw-string-literal.cpp

Index: test/clang-tidy/modernize-raw-string-literal.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-raw-string-literal.cpp
@@ -0,0 +1,123 @@
+// RUN: %check_clang_tidy %s modernize-raw-string-literal %t
+
+char const *const BackSlash{"goink\\frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: escaped string literal can be written as a raw string literal [modernize-raw-string-literal]
+// CHECK-FIXES: {{^}}char const *const BackSlash{R"(goink\frob)"};{{$}}
+
+char const *const PlainLiteral("plain literal");
+
+// Non-printable ASCII characters.
+char const *const Nul{"goink\\\000"};
+char const *const Soh{"goink\\\001"};
+char const *const Stx{"goink\\\002"};
+char const *const Etx{"goink\\\003"};
+char const *const Enq{"goink\\\004"};
+char const *const Ack{"goink\\\005"};
+char const *const Bell{"goink\\\afrob"};
+char const *const BackSpace{"goink\\\bfrob"};
+char const *const HorizontalTab{"goink\\\tfrob"};
+char const *const NewLine{"goink\nfrob"};
+char const *const VerticalTab{"goink\\\vfrob"};
+char const *const FormFeed{"goink\\\ffrob"};
+char const *const CarraigeReturn{"goink\\\rfrob"};
+char const *const So{"goink\\\016"};
+char const *const Si{"goink\\\017"};
+char const *const Dle{"goink\\\020"};
+char const *const Dc1{"goink\\\021"};
+char const *const Dc2{"goink\\\022"};
+char const *const Dc3{"goink\\\023"};
+char const *const Dc4{"goink\\\024"};
+char const *const Nak{"goink\\\025"};
+char const *const Syn{"goink\\\026"};
+char const *const Etb{"goink\\\027"};
+char const *const Can{"goink\\\030"};
+char const *const Em{"goink\\\031"};
+char const *const Sub{"goink\\\032"};
+char const *const Esc{"goink\\\033"};
+char const *const Fs{"goink\\\034"};
+char const *const Gs{"goink\\\035"};
+char const *const Rs{"goink\\\036"};
+char const *const Us{"goink\\\037"};
+char const *const HexNonPrintable{"\\\x03"};
+char const *const Delete{"\\\177"};
+
+char const *const TrailingSpace{"A line \\with space. \n"};
+char const *const TrailingNewLine{"A single \\line.\n"};
+char const *const AlreadyRaw{R"(foobie\\bletch)"};
+char const *const UTF8Literal{u8"foobie\\bletch"};
+char const *const UTF8RawLiteral{u8R"(foobie\\bletch)"};
+char16_t const *const UTF16Literal{u"foobie\\bletch"};
+char16_t const *const UTF16RawLiteral{uR"(foobie\\bletch)"};
+char32_t const *const UTF32Literal{U"foobie\\bletch"};
+char32_t const *const UTF32RawLiteral{UR"(foobie\\bletch)"};
+wchar_t const *const WideLiteral{L"foobie\\bletch"};
+wchar_t const *const WideRawLiteral{LR"(foobie\\bletch)"};
+
+char const *const SingleQuote{"goink\'frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
+// CHECK-XFIXES: {{^}}char const *const SingleQuote{R"(goink'frob)"};{{$}}
+
+char const *const DoubleQuote{"goink\"frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const DoubleQuote{R"(goink"frob)"};{{$}}
+
+char const *const QuestionMark{"goink\?frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const QuestionMark{R"(goink?frob)"};{{$}}
+
+char const *const RegEx{"goink\\(one|two\\)\\?.*\\nfrob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const RegEx{R"(goink\(one|two\)\\\?.*\nfrob)"};{{$}}
+
+char const *const Path{"C:\\Program Files\\Vendor\\Application\\Application.exe"};
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const Path{R"(C:\Program Files\Vendor\Application\Application.exe)"};{{$}}
+
+char const *const ContainsSentinel{"who\\ops)\""};
+// CHECK-MESSAGES: :[[@LINE-1]]:36: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const ContainsSentinel{R"lit(who\ops)")lit"};{{$}}
+
+char const *const ContainsDelim{"whoops)\")lit\""};
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const ContainsDelim{R"lit1(whoops)")lit")lit1"};{{$}}
+
+char const *const OctalPrintable{"\100\\"};
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const 

Re: [PATCH] D15332: new clang-tidy checker readability-non-const-parameter

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:116-134
@@ +115,21 @@
+
+// CHECK-MESSAGES: :[[@LINE+1]]:18: warning: parameter 'p' can be const
+int return1(int *p) {
+  // CHECK-FIXES: {{^}}int return1(const int *p) {{{$}}
+  return *p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: parameter 'p' can be const
+const int *return2(int *p) {
+  // CHECK-FIXES: {{^}}const int *return2(const int *p) {{{$}}
+  return p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: parameter 'p' can be const
+const int *return3(int *p) {
+  // CHECK-FIXES: {{^}}const int *return3(const int *p) {{{$}}
+  return p + 1;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:27: warning: parameter 'p' can be const
+const char *return4(char *p) {

danielmarjamaki wrote:
> rsmith wrote:
> > The wording of this warning, and the name of the check, are highly 
> > misleading.
> > 
> > It's *not* the parameter that can be const, it's the parameter's *pointee*.
> I understand. Figuring out a better message is not easy. I and a collegue 
> brain stormed:
> 
> pointer parameter 'p' could be declared with const 'const int *p'
> 
> pointer parameter 'p' could be pointer to const
> 
> pointer parameter 'p' declaration could be pointer to const
> 
> missing const in pointer parameter 'p' declaration
> 
> declaration of pointer parameter 'p' could be 'const int *p'
> 
> Do you think any of these would be good?
> 
Regarding the name of the check, I can't think of anything better only things 
that are longer but aren't substantially more clear in revealing the intent of 
the check.  IMO, you don't blindly run clang-tidy checks based on the name, you 
run them after reading what they do and deciding if you want that 
transformation or not.

In order of my preference:

  - pointer parameter 'p' could be pointer to const
  - declaration of pointer parameter 'p' could be 'const int *p'

I might use the word 'can' instead of the world 'could', but that's a very 
minor quibble.


http://reviews.llvm.org/D15332



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

Re: [PATCH] D18191: [clang-tidy] Add check for function parameters that are const& to builtin types

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.
LegalizeAdulthood added a comment.

There is utility in the definition of a function in saying that an argument is 
`const int i` instead of `int i`.  The const-ness declares the intent that this 
local variable is not going to be modified.  However, there is that oddity in 
C++ that allows a declaration to say `void f(int i);` and the implementation to 
say `void f(const int i) { ... }`.

I think I would like the fixit to preserve the const-ness of the argument while 
still stripping it of it's reference.


http://reviews.llvm.org/D18191



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

[PATCH] D18263: [clang-tools-extra] Add release notes documentation

[Richard via cfe-commits]

LegalizeAdulthood created this revision.
LegalizeAdulthood added a reviewer: alexfh.
LegalizeAdulthood added a subscriber: cfe-commits.

Add a file for Extra Clang Tools release notes, based on 3.9 release notes file 
for Clang.

http://reviews.llvm.org/D18263

Files:
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/index.rst

Index: docs/index.rst
===
--- docs/index.rst
+++ docs/index.rst
@@ -10,6 +10,10 @@
 Welcome to the clang-tools-extra project which contains extra tools built using
 Clang's tooling API's.
 
+.. toctree::
+   :maxdepth: 1
+
+   ReleaseNotes
 
 Contents
 
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -102,3 +102,4 @@
readability-redundant-string-init
readability-simplify-boolean-expr
readability-uniqueptr-delete-release
+   performance-unnecessary-copy-initialization.rst
Index: docs/ReleaseNotes.rst
===
--- /dev/null
+++ docs/ReleaseNotes.rst
@@ -0,0 +1,72 @@
+=
+Extra Clang Tools 3.9 (In-Progress) Release Notes
+=
+
+.. contents::
+   :local:
+   :depth: 2
+
+Written by the `LLVM Team `_
+
+.. warning::
+
+   These are in-progress notes for the upcoming Clang 3.9 release. You may
+   prefer the `Clang 3.8 Release Notes
+   `_.
+
+Introduction
+
+
+This document contains the release notes for the Extra Clang Tools, part of the
+Clang release 3.9.  Here we describe the status of the Extra Clang Tools in some
+detail, including major improvements from the previous release and new feature
+work. For the general Clang release notes, see `the Clang documentation
+`_.  All LLVM
+releases may be downloaded from the `LLVM releases web
+site `_.
+
+For more information about Clang or LLVM, including information about
+the latest release, please check out the main please see the `Clang Web
+Site `_ or the `LLVM Web
+Site `_.
+
+Note that if you are reading this file from a Subversion checkout or the
+main Clang web page, this document applies to the *next* release, not
+the current one. To see the release notes for a specific release, please
+see the `releases page `_.
+
+What's New in Extra Clang Tools 3.9?
+
+
+Some of the major new features and improvements to Extra Clang Tools are listed
+here. Generic improvements to Extra Clang Tools as a whole or to its underlying
+infrastructure are described first, followed by tool-specific sections.
+
+Major New Features
+--
+
+- Feature1...
+
+Improvements to ``clang-query``
+^^^
+
+The improvements are...
+
+Improvements to ``clang-rename``
+
+
+The improvements are...
+
+Improvements to ``clang-tidy``
+^^
+
+``clang-tidy``'s checks are constantly being improved to catch more issues,
+explain them more clearly, and provide more accurate fix-its for the issues
+identified.  The improvements since the 3.8 release include:
+
+-  ...
+
+Improvements to ``modularize``
+^^
+
+The improvements are...
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18263: [clang-tools-extra] Add release notes documentation

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 51013.
LegalizeAdulthood added a comment.

Sort check names in list


http://reviews.llvm.org/D18263

Files:
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/index.rst

Index: docs/index.rst
===
--- docs/index.rst
+++ docs/index.rst
@@ -10,6 +10,10 @@
 Welcome to the clang-tools-extra project which contains extra tools built using
 Clang's tooling API's.
 
+.. toctree::
+   :maxdepth: 1
+
+   ReleaseNotes
 
 Contents
 
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -88,6 +88,7 @@
performance-faster-string-find
performance-for-range-copy
performance-implicit-cast-in-loop
+   performance-unnecessary-copy-initialization.rst
readability-braces-around-statements
readability-container-size-empty
readability-else-after-return
Index: docs/ReleaseNotes.rst
===
--- /dev/null
+++ docs/ReleaseNotes.rst
@@ -0,0 +1,72 @@
+=
+Extra Clang Tools 3.9 (In-Progress) Release Notes
+=
+
+.. contents::
+   :local:
+   :depth: 2
+
+Written by the `LLVM Team `_
+
+.. warning::
+
+   These are in-progress notes for the upcoming Clang 3.9 release. You may
+   prefer the `Clang 3.8 Release Notes
+   `_.
+
+Introduction
+
+
+This document contains the release notes for the Extra Clang Tools, part of the
+Clang release 3.9.  Here we describe the status of the Extra Clang Tools in some
+detail, including major improvements from the previous release and new feature
+work. For the general Clang release notes, see `the Clang documentation
+`_.  All LLVM
+releases may be downloaded from the `LLVM releases web
+site `_.
+
+For more information about Clang or LLVM, including information about
+the latest release, please check out the main please see the `Clang Web
+Site `_ or the `LLVM Web
+Site `_.
+
+Note that if you are reading this file from a Subversion checkout or the
+main Clang web page, this document applies to the *next* release, not
+the current one. To see the release notes for a specific release, please
+see the `releases page `_.
+
+What's New in Extra Clang Tools 3.9?
+
+
+Some of the major new features and improvements to Extra Clang Tools are listed
+here. Generic improvements to Extra Clang Tools as a whole or to its underlying
+infrastructure are described first, followed by tool-specific sections.
+
+Major New Features
+--
+
+- Feature1...
+
+Improvements to ``clang-query``
+^^^
+
+The improvements are...
+
+Improvements to ``clang-rename``
+
+
+The improvements are...
+
+Improvements to ``clang-tidy``
+^^
+
+``clang-tidy``'s checks are constantly being improved to catch more issues,
+explain them more clearly, and provide more accurate fix-its for the issues
+identified.  The improvements since the 3.8 release include:
+
+-  ...
+
+Improvements to ``modularize``
+^^
+
+The improvements are...
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18136: boost-use-to-string check

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: clang-tidy/boost/UseToStringCheck.cpp:56
@@ +55,3 @@
+  const auto *MatchedToString = Result.Nodes.getNodeAs("to_string");
+  auto Q = Result.Nodes.getNodeAs("char_type")->getAsType();
+

Q seems pretty obtuse to me as an identifier.  I normally think of template 
arguments as `T`, if we are going to use a single letter.  Otherwise an 
intention revealing name would be better.  Something like `argType` perhaps?


http://reviews.llvm.org/D18136



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

Re: [PATCH] D18191: [clang-tidy] Add check for function parameters that are const& to builtin types

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: test/clang-tidy/misc-const-ref-builtin.cpp:32-34
@@ +31,4 @@
+
+void f(const int& i, const int& j);
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: const reference to 'int' at 
parameter 'i' in function 'f' [misc-const-ref-builtin]
+// CHECK-MESSAGES: :[[@LINE-2]]:33: warning: const reference to 'int' at 
parameter 'j' in function 'f' [misc-const-ref-builtin]

I see tests here only for declarations, and not definitions of functions.

I also don't see any tests for:

  - member functions
  - template member functions
  - template specializations
  - tests where function signatures result from macro expansion, either in the 
body of a macro or as a macro argument



http://reviews.llvm.org/D18191



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

Re: [PATCH] D18238: [clang-tidy] Fix clang-tidy crashes when using -fdelayed-template-parsing.

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.
LegalizeAdulthood added a comment.

LGTM


http://reviews.llvm.org/D18238



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

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D17482#372677, @alexfh wrote:

> Sorry for leaving this without attention for a while.
>
> I'm somewhat concerned about this change. It's adding a certain level of 
> complexity, but doesn't cover some less trivial cases like handling of 
> multiple headers.


In the context of a clang-tidy check, what would be gained by verifying changes 
made to multiple headers that isn't already tested by verifying changes made to 
a single header?

> Can you take a look at existing tests and say how many times this feature is 
> going to be used, if it gets added to the testing script?


At least two of the checks I've already added have complaints that they don't 
work on headers.

While this change was waiting to be reviewed, another check was added that made 
changes to headers but had no way to verify them.


http://reviews.llvm.org/D17482



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

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 51105.
LegalizeAdulthood marked an inline comment as done.
LegalizeAdulthood added a comment.

Update from comments.
Update documentation to reflect changes in the implementation.

I do not have commit access.

Patch by Richard Thomson


http://reviews.llvm.org/D16529

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/RawStringLiteralCheck.cpp
  clang-tidy/modernize/RawStringLiteralCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-raw-string-literal.rst
  test/clang-tidy/modernize-raw-string-literal-delimiter.cpp
  test/clang-tidy/modernize-raw-string-literal.cpp

Index: test/clang-tidy/modernize-raw-string-literal.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-raw-string-literal.cpp
@@ -0,0 +1,123 @@
+// RUN: %check_clang_tidy %s modernize-raw-string-literal %t
+
+char const *const BackSlash{"goink\\frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: escaped string literal can be written as a raw string literal [modernize-raw-string-literal]
+// CHECK-FIXES: {{^}}char const *const BackSlash{R"(goink\frob)"};{{$}}
+
+char const *const PlainLiteral("plain literal");
+
+// Non-printable ASCII characters.
+char const *const Nul{"goink\\\000"};
+char const *const Soh{"goink\\\001"};
+char const *const Stx{"goink\\\002"};
+char const *const Etx{"goink\\\003"};
+char const *const Enq{"goink\\\004"};
+char const *const Ack{"goink\\\005"};
+char const *const Bell{"goink\\\afrob"};
+char const *const BackSpace{"goink\\\bfrob"};
+char const *const HorizontalTab{"goink\\\tfrob"};
+char const *const NewLine{"goink\nfrob"};
+char const *const VerticalTab{"goink\\\vfrob"};
+char const *const FormFeed{"goink\\\ffrob"};
+char const *const CarraigeReturn{"goink\\\rfrob"};
+char const *const So{"goink\\\016"};
+char const *const Si{"goink\\\017"};
+char const *const Dle{"goink\\\020"};
+char const *const Dc1{"goink\\\021"};
+char const *const Dc2{"goink\\\022"};
+char const *const Dc3{"goink\\\023"};
+char const *const Dc4{"goink\\\024"};
+char const *const Nak{"goink\\\025"};
+char const *const Syn{"goink\\\026"};
+char const *const Etb{"goink\\\027"};
+char const *const Can{"goink\\\030"};
+char const *const Em{"goink\\\031"};
+char const *const Sub{"goink\\\032"};
+char const *const Esc{"goink\\\033"};
+char const *const Fs{"goink\\\034"};
+char const *const Gs{"goink\\\035"};
+char const *const Rs{"goink\\\036"};
+char const *const Us{"goink\\\037"};
+char const *const HexNonPrintable{"\\\x03"};
+char const *const Delete{"\\\177"};
+
+char const *const TrailingSpace{"A line \\with space. \n"};
+char const *const TrailingNewLine{"A single \\line.\n"};
+char const *const AlreadyRaw{R"(foobie\\bletch)"};
+char const *const UTF8Literal{u8"foobie\\bletch"};
+char const *const UTF8RawLiteral{u8R"(foobie\\bletch)"};
+char16_t const *const UTF16Literal{u"foobie\\bletch"};
+char16_t const *const UTF16RawLiteral{uR"(foobie\\bletch)"};
+char32_t const *const UTF32Literal{U"foobie\\bletch"};
+char32_t const *const UTF32RawLiteral{UR"(foobie\\bletch)"};
+wchar_t const *const WideLiteral{L"foobie\\bletch"};
+wchar_t const *const WideRawLiteral{LR"(foobie\\bletch)"};
+
+char const *const SingleQuote{"goink\'frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
+// CHECK-XFIXES: {{^}}char const *const SingleQuote{R"(goink'frob)"};{{$}}
+
+char const *const DoubleQuote{"goink\"frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const DoubleQuote{R"(goink"frob)"};{{$}}
+
+char const *const QuestionMark{"goink\?frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const QuestionMark{R"(goink?frob)"};{{$}}
+
+char const *const RegEx{"goink\\(one|two\\)\\?.*\\nfrob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const RegEx{R"(goink\(one|two\)\\\?.*\nfrob)"};{{$}}
+
+char const *const Path{"C:\\Program Files\\Vendor\\Application\\Application.exe"};
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const Path{R"(C:\Program Files\Vendor\Application\Application.exe)"};{{$}}
+
+char const *const ContainsSentinel{"who\\ops)\""};
+// CHECK-MESSAGES: :[[@LINE-1]]:36: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const ContainsSentinel{R"lit(who\ops)")lit"};{{$}}
+
+char const *const ContainsDelim{"whoops)\")lit\""};
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const ContainsDelim{R"lit1(whoops)")lit")lit1"};{{$}}
+
+char const *const 

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D17482#372677, @alexfh wrote:

> Can you [...] say how many times this feature is going to be used, if it gets 
> added to the testing script?


Since you can't verify changes against headers right now, noone has bothered to 
add test cases for their checks against headers.

However, I think a case can be made that every check potentially operates 
against headers and that they should be verified as such.  But, since you can't 
do this in the build right now, noone has written such tests.


http://reviews.llvm.org/D17482



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

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

test/clang-tidy/misc-unused-parameters.cpp currently does a hack to verify 
headers by committing the manually fixed header and diffing it:

  // RUN: echo "static void staticFunctionHeader(int i) {}" > %T/header.h
  // RUN: echo "static void staticFunctionHeader(int  /*i*/) {}" > 
%T/header-fixed.h
  // RUN: %check_clang_tidy %s misc-unused-parameters %t -- -header-filter='.*' 
-- -std=c++11 -fno-delayed-template-parsing
  // RUN: diff %T/header.h %T/header-fixed.h
  
  #include "header.h"
  // CHECK-MESSAGES: header.h:1:38: warning

I think this would be much cleaner with `header.h` as a regularly comitted file 
with `CHECK-MESSAGES` and `CHECK-FIXES` lines in that file.


http://reviews.llvm.org/D17482



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

Re: [PATCH] D18263: [clang-tools-extra] Add release notes documentation

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 51102.
LegalizeAdulthood marked 2 inline comments as done.
LegalizeAdulthood added a comment.

Update from review comments.

I do not have commit access.

Patch by Richard Thomson


http://reviews.llvm.org/D18263

Files:
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/index.rst

Index: docs/index.rst
===
--- docs/index.rst
+++ docs/index.rst
@@ -10,6 +10,10 @@
 Welcome to the clang-tools-extra project which contains extra tools built using
 Clang's tooling API's.
 
+.. toctree::
+   :maxdepth: 1
+
+   ReleaseNotes
 
 Contents
 
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -88,6 +88,7 @@
performance-faster-string-find
performance-for-range-copy
performance-implicit-cast-in-loop
+   performance-unnecessary-copy-initialization
readability-braces-around-statements
readability-container-size-empty
readability-else-after-return
Index: docs/ReleaseNotes.rst
===
--- /dev/null
+++ docs/ReleaseNotes.rst
@@ -0,0 +1,71 @@
+=
+Extra Clang Tools 3.9 (In-Progress) Release Notes
+=
+
+.. contents::
+   :local:
+   :depth: 2
+
+Written by the `LLVM Team `_
+
+.. warning::
+
+   These are in-progress notes for the upcoming Clang 3.9 release. You may
+   prefer the `Clang 3.8 Release Notes
+   `_.
+
+Introduction
+
+
+This document contains the release notes for the Extra Clang Tools, part of the
+Clang release 3.9.  Here we describe the status of the Extra Clang Tools in some
+detail, including major improvements from the previous release and new feature
+work. For the general Clang release notes, see `the Clang documentation
+`_.  All LLVM
+releases may be downloaded from the `LLVM releases web
+site `_.
+
+For more information about Clang or LLVM, including information about
+the latest release, please see the `Clang Web Site `_ or
+the `LLVM Web Site `_.
+
+Note that if you are reading this file from a Subversion checkout or the
+main Clang web page, this document applies to the *next* release, not
+the current one. To see the release notes for a specific release, please
+see the `releases page `_.
+
+What's New in Extra Clang Tools 3.9?
+
+
+Some of the major new features and improvements to Extra Clang Tools are listed
+here. Generic improvements to Extra Clang Tools as a whole or to its underlying
+infrastructure are described first, followed by tool-specific sections.
+
+Major New Features
+--
+
+- Feature1...
+
+Improvements to ``clang-query``
+^^^
+
+The improvements are...
+
+Improvements to ``clang-rename``
+
+
+The improvements are...
+
+Improvements to ``clang-tidy``
+^^
+
+``clang-tidy``'s checks are constantly being improved to catch more issues,
+explain them more clearly, and provide more accurate fix-its for the issues
+identified.  The improvements since the 3.8 release include:
+
+-  ...
+
+Improvements to ``modularize``
+^^
+
+The improvements are...
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18191: [clang-tidy] Add check for function parameters that are const& to builtin types

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D18191#377686, @sdowney wrote:

> In http://reviews.llvm.org/D18191#376471, @LegalizeAdulthood wrote:
>
> > There is utility in the definition of a function in saying that an argument 
> > is `const int i` instead of `int i`.  The const-ness declares the intent 
> > that this local variable is not going to be modified.  However, there is 
> > that oddity in C++ that allows a declaration to say `void f(int i);` and 
> > the implementation to say `void f(const int i) { ... }`.
> >
> > I think I would like the fixit to preserve the const-ness of the argument 
> > while still stripping it of it's reference.
>
>
> The usual pattern for that is to have the definition use const, and the 
> declaration not, since it's an implementation detail if the passed parameter 
> is modified. And, unfortunately, I'm stuck with one compiler that mangles 
> those two differently, causing link errors.
>
> Having void foo(const int i) in a declaration makes me suspicious, since the 
> const is meaningless.
>
> I could make this an option? If the option is set, emit 'const type' if the 
> Decl hasBody?


It seems reasonable to do this as a refinement after this check is in place.


http://reviews.llvm.org/D18191



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

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Another week without reviews...

These changes were already split from a previous changeset without being 
reviewed properly.

Squeak squeak squeak.


http://reviews.llvm.org/D17482



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

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

I do not have commit access, so I will need someone to commit this for me.

Patch by Richard Thomson

thanks.



Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:82
@@ +81,3 @@
+? std::string{R"lit()")lit"}
+: (")" + Delimiter + R"(")")) != StringRef::npos;
+}

aaron.ballman wrote:
> This is a wonderful demonstration of why I hate raw string literals on 
> shorter strings. I had to stare at that raw string for quite some time to 
> figure it out. :-(
I used a raw string literal here because that's what this check would have 
recommended on this code :-).

However, your feedback is useful.  If I subsequently enhance this check with a 
parameter that says the minimum length a string literal must have before it is 
to be transformed into a raw string literal and the default value for that 
parameter is something like 5, would that address your concern?


http://reviews.llvm.org/D16529



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

Re: [PATCH] D7982: Add readability-duplicate-include check to clang-tidy

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

I need to update from review comments and upload a new diff.


http://reviews.llvm.org/D7982



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

Re: [PATCH] D10013: Refactor: Simplify boolean conditional return statements in lib/Driver

[Richard via cfe-commits]

LegalizeAdulthood abandoned this revision.
LegalizeAdulthood added a comment.

Discarding due to inaction by reviewers.


http://reviews.llvm.org/D10013



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

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Squeak.  This has been waiting on review for over two weeks


http://reviews.llvm.org/D16529



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

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Squeak


http://reviews.llvm.org/D17482



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

Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.


Comment at: docs/clang-tidy/checks/misc-suspicious-semicolon.rst:48-49
@@ +47,4 @@
+
+while(readWhitespace());
+  Token t = readNextToken();
+

In this documentation, can we please format code so that control structures 
don't look like function calls?  By this I mean write:
```
if (x >=y);
  x -= y;

while (readwhitespace());
  Token t = readNextToken();
```
instead of
```
if(x >=y);
  x -= y;

while(readwhitespace());
  Token t = readNextToken();
```



Repository:
  rL LLVM

http://reviews.llvm.org/D16535



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

Re: [PATCH] D17446: ASTMatchers: add new statement/decl matchers

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.


Comment at: docs/LibASTMatchersReference.html:512
@@ +511,3 @@
+Matcherhttp://clang.llvm.org/doxygen/classclang_1_1Stmt.html;>StmtaddrLabelExprMatcherhttp://clang.llvm.org/doxygen/classclang_1_1AddrLabelExpr.html;>AddrLabelExpr...
+Matches address of 
label statements (GNU extension).
+

Can we put `gnu` in the name of matchers that only match GNU extensions?


Comment at: docs/LibASTMatchersReference.html:544
@@ +543,3 @@
+Matcherhttp://clang.llvm.org/doxygen/classclang_1_1Stmt.html;>StmtatomicExprMatcherhttp://clang.llvm.org/doxygen/classclang_1_1AtomicExpr.html;>AtomicExpr...
+Matches atomic builtins.
+

Please provide an example here.


Comment at: docs/LibASTMatchersReference.html:549
@@ +548,3 @@
+Matcherhttp://clang.llvm.org/doxygen/classclang_1_1Stmt.html;>StmtbinaryConditionalOperatorMatcherhttp://clang.llvm.org/doxygen/classclang_1_1BinaryConditionalOperator.html;>BinaryConditionalOperator...
+Matches 
binary conditional operator expressions (GNU extension).
+

See above re: gnu specific extensions.  When I read the name of this matcher, I 
expected it to be matching `==`, `!=`, `<=`, etc.


Comment at: docs/LibASTMatchersReference.html:1120
@@ +1119,3 @@
+Matcherhttp://clang.llvm.org/doxygen/classclang_1_1Stmt.html;>StmtopaqueValueExprMatcherhttp://clang.llvm.org/doxygen/classclang_1_1OpaqueValueExpr.html;>OpaqueValueExpr...
+Matches opaque 
value expressions.
+

The docs say that OpaqueValueExpr doesn't correspond to concrete syntax, so why 
would we have a matcher for it?


Comment at: docs/LibASTMatchersReference.html:1140
@@ +1139,3 @@
+Given
+  templatetypename T class X { void f() { X x(*this); } };
+parenListExpr()

Can we have a simpler example?  For instance, I can't see that `ParenListExpr` 
has anything to do with templates.


Comment at: docs/LibASTMatchersReference.html:1149
@@ +1148,3 @@
+
+Example: Matches __func__)
+  printf("%s", __func__);

Does it actually match the closing paren?


Comment at: docs/LibASTMatchersReference.html:1175
@@ +1174,3 @@
+Matcherhttp://clang.llvm.org/doxygen/classclang_1_1Stmt.html;>StmtstmtExprMatcherhttp://clang.llvm.org/doxygen/classclang_1_1StmtExpr.html;>StmtExpr...
+Matches GNU statement 
expression.
+

Ditto re: gnu extension


Comment at: docs/LibASTMatchersReference.html:1752
@@ +1751,3 @@
+Matcherhttp://clang.llvm.org/doxygen/classclang_1_1CXXConstructExpr.html;>CXXConstructExprrequiresZeroInitialization
+Matches 
a constructor call expression which requires
+zero initialization.

Please provide an example.


http://reviews.llvm.org/D17446



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

Re: [PATCH] D17484: [clang-tidy] introduce modernize-deprecated-headers check

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D17484#358617, @alexfh wrote:

> Thank you for this check! Mostly looks good, but there are a number of style 
> nits.
>
> The most important question to this check is that the standard doesn't 
> guarantee that the C++ headers declare all the same functions **in the global 
> namespace** (at least, this is how I understand the section of the standard 
> you quoted).


Oh crap, I totally forgot about this.  Yes, I believe the weasel wording in the 
standard is that an implementation may put the symbols in the global namespace 
as well as inside `std` when the C++ header is used, but is not required to put 
them in the global namespace.  They are required to put them in namespace `std` 
when the C++ header form is used.

So if you switch to C++ headers, you code may still compile, but it is 
implementation dependent.

If you use the C headers, you can't prefix symbols with `std` because they 
won't be there.

In other discussions of this topic, it was considered best to make both changes 
at the same time: switch to the C++ header and decorate the symbols with `std` 
prefix.

It would be reasonable for this check to insert a `using namespace std;` at the 
top of the translation unit after all the `#include` lines as a means of 
preserving meaning without trying to identify every symbol that needs `std` on 
it.


http://reviews.llvm.org/D17484



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

Re: [PATCH] D17484: [clang-tidy] introduce modernize-deprecated-headers check

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Other than a few nits, LGTM.



Comment at: clang-tidy/modernize/DeprecatedHeadersCheck.cpp:54
@@ +53,3 @@
+: Check(Check), LangOpts(LangOpts) {
+  CStyledHeaderToCxx["assert.h"] = "cassert";
+  CStyledHeaderToCxx["float.h"] = "cfloat";

Can't we use C++11 brace initialization here instead of repeated assignments?


Comment at: docs/clang-tidy/checks/modernize-deprecated-headers.rst:33
@@ +32,3 @@
+
+The following headers were deprecated before C++ 11:
+

Can we sort these lists of includes?  Otherwise it's hard to find a specific 
include in the list.


Comment at: test/clang-tidy/modernize-deprecated-headers-cxx11.cpp:3
@@ +2,3 @@
+
+#include "assert.h"
+#include "float.h"

Can we sort the includes in the test files too please?


http://reviews.llvm.org/D17484



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

[PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood created this revision.
LegalizeAdulthood added a reviewer: alexfh.
LegalizeAdulthood added a subscriber: cfe-commits.

This changeset improves check_clang_tidy.py to allow chnages made by a check to 
a header to be verified.

A new argument `--header-filter` allows the test file to specify the header 
that will be modified by the check.

`check_clang_tidy.py` will then:

- Copy the named header to the same temporary directory containing the source 
file
- Scrub `CHECK-FIXES` and `CHECK-FIXES` text from the copied header
- Run the `clang-tidy`  on the scrubbed source files with:
  - `-header-filter` and the name of the copied header file.
  -  `-I .` to specify the temporary directory in the include search path
- Verify messages and fixes on the source file.
- Verify messages and fixes on the header file.
- In the case of failure, report differences on the header and the source file.

http://reviews.llvm.org/D17482

Files:
  test/clang-tidy/check_clang_tidy.py
  test/clang-tidy/modernize-redundant-void-arg.cpp
  test/clang-tidy/modernize-redundant-void-arg.h

Index: test/clang-tidy/modernize-redundant-void-arg.h
===
--- /dev/null
+++ test/clang-tidy/modernize-redundant-void-arg.h
@@ -0,0 +1,8 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H
+#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H
+
+extern int foo(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: redundant void argument list in function declaration [modernize-redundant-void-arg]
+// CHECK-FIXES: {{^}}extern int foo();{{$}}
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H
Index: test/clang-tidy/modernize-redundant-void-arg.cpp
===
--- test/clang-tidy/modernize-redundant-void-arg.cpp
+++ test/clang-tidy/modernize-redundant-void-arg.cpp
@@ -1,4 +1,6 @@
-// RUN: %check_clang_tidy %s modernize-redundant-void-arg %t
+// RUN: %check_clang_tidy --header-filter=modernize-redundant-void-arg.h %s modernize-redundant-void-arg %t
+
+#include "modernize-redundant-void-arg.h"
 
 #define NULL 0
 
Index: test/clang-tidy/check_clang_tidy.py
===
--- test/clang-tidy/check_clang_tidy.py
+++ test/clang-tidy/check_clang_tidy.py
@@ -25,6 +25,7 @@
 """
 
 import argparse
+import os
 import re
 import subprocess
 import sys
@@ -35,16 +36,98 @@
 f.write(text)
 f.truncate()
 
+
+# Remove the contents of the CHECK lines to avoid CHECKs matching on
+# themselves.  We need to keep the comments to preserve line numbers while
+# avoiding empty lines which could potentially trigger formatting-related
+# checks.
+def clean_text(input_text):
+  return re.sub('// *CHECK-[A-Z-]*:[^\r\n]*', '//', input_text)
+
+
+def write_cleaned_header(input_file_path, temp_file_name, header_file_name):
+  src_path = os.path.join(os.path.dirname(input_file_path), header_file_name)
+  fixed_path = os.path.join(os.path.dirname(temp_file_name), header_file_name)
+  with open(src_path, 'r') as input_file:
+cleaned_text = clean_text(input_file.read())
+  cleaned_path = fixed_path + '.orig'
+  write_file(cleaned_path, cleaned_text)
+  write_file(fixed_path, cleaned_text)
+  return cleaned_path, fixed_path, src_path
+
+
+def separate_output(clang_tidy_output, header_file_name, input_file_name):
+  input_file_name = os.path.basename(input_file_name)
+  input_file_output = ''
+  header_file_output = ''
+  input_messages = True
+  for line in clang_tidy_output.splitlines(True):
+if input_messages:
+  if header_file_name in line:
+input_messages = False
+header_file_output += line
+  else:
+input_file_output += line
+else:
+  if input_file_name in line:
+input_messages = True
+input_file_output += line
+  else:
+header_file_output += line
+  return header_file_output, input_file_output
+
+
+def try_run(args):
+  try:
+process_output = \
+  subprocess.check_output(args, stderr=subprocess.STDOUT).decode()
+  except subprocess.CalledProcessError as e:
+print('%s failed:\n%s' % (' '.join(args), e.output.decode()))
+raise
+  return process_output
+
+
+def check_output_for_messages(messages_file, input_file_name):
+  try_run(
+['FileCheck', '-input-file=' + messages_file, input_file_name,
+   '-check-prefix=CHECK-MESSAGES',
+   '-implicit-check-not={{warning|error}}:'])
+
+
+def check_output_for_fixes(temp_file_name, input_file_name):
+  try_run(
+  ['FileCheck', '-input-file=' + temp_file_name, input_file_name,
+   '-check-prefix=CHECK-FIXES', '-strict-whitespace'])
+
+
+def has_checks(input_text):
+  has_check_fixes = input_text.find('CHECK-FIXES') >= 0
+  has_check_messages = input_text.find('CHECK-MESSAGES') >= 0
+  return has_check_fixes, has_check_messages
+
+
+def 

Re: [PATCH] D10013: Refactor: Simplify boolean conditional return statements in lib/Driver

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

I do not have commit access, so someone else will need to commit this.  Let me 
know if it needs rebasing.

Patch by Richard Thomson.


http://reviews.llvm.org/D10013



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

Re: [PATCH] D16012: Carry raw string literal information through to the AST StringLiteral representation

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D16012#351827, @rsmith wrote:

> I'm definitely sympathetic to making `StringLiteralParser` expose information 
> [...]


I was unaware of this class; so far I have only studied the classes appearing 
in the AST.

I did notice that the AST shows string literals after concatenation (which 
makes perfect sense) and sometimes for refactoring you want to treat the 
individual string literal chunks separately.  My clang-tidy check for raw 
string literals is probably confused by literal concatenation in the AST, so I 
will go add some tests for those cases.

One area of clang-tidy/refactoring tooling code development that is pretty 
opaque at the moment is when you should dip down to relexing/reparsing the 
source text and when you use the AST.  So far most (all?) of the documentation 
on guiding developers to writing such tools talks almost exclusively about the 
AST.  Anything we can do to simplify lexing/parsing tasks when you need to dip 
lower than the AST would be most welcome.


http://reviews.llvm.org/D16012



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

Re: [PATCH] D15332: new clang-tidy checker readability-non-const-parameter

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:3
@@ +2,3 @@
+
+// Currently the checker only warns about pointer arguments.
+//

danielmarjamaki wrote:
> LegalizeAdulthood wrote:
> > How hard is it to extend it to references?
> > 
> > Certainly the confusion about what is const is easier to resolve in the 
> > case of references because the references themselves are immutable.
> If a "int &" reference parameter is not written then probably it's better to 
> pass it by value than making it const. I would prefer that unless it has to 
> use "int &" to comply with some interface.
> 
> I realize that the same can be said about pointers. If there is a "int *p" 
> and you just read the value that p points at .. maybe sometimes it would be 
> preferable to pass it by value.
I thought the point of this check was to identify stuff passed by 
reference/pointer that could instead be passed by const reference/pointer.

Passing a read-only number type (`short`, `char`, `int`, `float`, `double`, 
etc.) parameter by pointer or by reference/pointer is a code smell, but this 
check isn't trying to identify that situation.

I'm more interested in things like:

```
void foo(std::string );
```

becoming

```
void foo(const std::string );
```

when `s` is treated as a read-only value within the implementation of `foo`.


http://reviews.llvm.org/D15332



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

Re: [PATCH] D15332: new clang-tidy checker readability-non-const-parameter

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:3
@@ +2,3 @@
+
+// Currently the checker only warns about pointer arguments.
+//

How hard is it to extend it to references?

Certainly the confusion about what is const is easier to resolve in the case of 
references because the references themselves are immutable.


http://reviews.llvm.org/D15332



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

Re: [PATCH] D15332: new clang-tidy checker readability-non-const-parameter

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.


Comment at: docs/clang-tidy/checks/readability-non-const-parameter.rst:15
@@ +14,3 @@
+
+  // warning here; p should be const
+  char f1(char *p) {

With pointers, there are always two layers of constness:


  - Whether the pointer itself is modified
  - Whether the data pointed to is modified

When you say "p should be const", I read that as the former.

I am pretty sure you intended the latter.  You should be more explicit in your 
documentation.  The ambiguity would have been resolved if you showed the 
"after" version of the code that would eliminate the warning.  This is 
semi-implied by your next example, but it's kinder to the reader to resolve 
this ambiguity immediately.




Comment at: docs/clang-tidy/checks/readability-non-const-parameter.rst:32
@@ +31,3 @@
+  // no warning; Technically, p can be const ("const struct S *p"). But making
+  // p const could be misleading. People might think that it's safe to pass
+  // const data to this function.

Here you are using "making p const" in the first sense I noted above.

It might help if you say "make `*p` const" when you mean that p points to 
constant data and use "making `p` const" when you mean that the pointer itself 
is unmodified.

There is also the wart in C++ that one can declare a function like this:

```
int f(char *p);
```

and define it like this:

```
int f(char *const p) {
  // ...
}
```

My sense is that this is pretty confusing to most programmers who really want 
the declaration and definition to be textually identical.


http://reviews.llvm.org/D15332



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

Re: [PATCH] D17244: [clang-tidy] readability-ternary-operator new check

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: docs/clang-tidy/checks/readability-ternary-operator.rst:10
@@ +9,3 @@
+And it gets transformed into::
+  (condition ? expression0 : expression1);
+

Why add the parentheses around the entire statement?  They seem to serve no 
purpose.

What happens if the `condition` is an expression with the same or lower 
precedence than the ternary operator?

Most people seem to agree that `(a || b) ? e1 : e2;` is more readable than `(a 
|| b ? e1 : e2);`


Comment at: docs/clang-tidy/checks/readability-ternary-operator.rst:13
@@ +12,3 @@
+Another situation when ternary operator would be suggested::
+  if (condition) return literal0; else return literal1;
+

[[ 
http://clang.llvm.org/extra/clang-tidy/checks/readability-simplify-boolean-expr.html
 | readability-simplify-boolean-expr ]] does this when the return type is 
`bool` and the return statements are returning `true` and `false`.

It also handles conditional assignment via ternary operator when the value 
assigned is of type `bool` and the resulting expressions are boolean constants.

If this check also starts modifying the same ternary operators, then we can 
have two checks fighting to make changes if both checks are applied on the same 
run of `clang-tidy`.  I'm unsure how best to resolve the conflict.  I don't 
know what `clang-tidy` would do in such instances.


Comment at: docs/clang-tidy/checks/readability-ternary-operator.rst:18
@@ +17,3 @@
+
+The autofixes are only suggested when no comments would be lost.
+

Are you also checking for intermediate preprocessor lines?

```
if (cond) {
#define BLAH 1
  return BLAH;
} else {
  return 0;
}
```


Comment at: docs/clang-tidy/checks/readability-ternary-operator.rst:24
@@ +23,2 @@
+both comments will be lost, therefore there will be no autofix suggested for
+such case. If you want the check to perform an autofix just remove the 
comments.

A slightly better wording:

If you want the check to perform an autofix, either reposition or remove the 
comments.


Comment at: test/clang-tidy/readability-ternary-operator.cpp:3
@@ +2,3 @@
+
+bool Condition = true;
+int Variable = 0;

You don't have any test cases where the condition is an expression with 
operators.  Please add some tests for this, particularly operators of the same 
or lower precedence than the ternary operator.


http://reviews.llvm.org/D17244



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

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 47927.
LegalizeAdulthood marked 4 inline comments as done.
LegalizeAdulthood added a comment.

Update from review comments.
Avoid some string concatenation when delimiter is empty.


http://reviews.llvm.org/D16529

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/RawStringLiteralCheck.cpp
  clang-tidy/modernize/RawStringLiteralCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-raw-string-literal.rst
  test/clang-tidy/modernize-raw-string-literal.cpp

Index: test/clang-tidy/modernize-raw-string-literal.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-raw-string-literal.cpp
@@ -0,0 +1,122 @@
+// RUN: %check_clang_tidy %s modernize-raw-string-literal %t
+
+char const *const BackSlash{"goink\\frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: escaped string literal can be written as a raw string literal [modernize-raw-string-literal]
+// CHECK-FIXES: {{^}}char const *const BackSlash{R"(goink\frob)"};{{$}}
+
+char const *const PlainLiteral("plain literal");
+
+// Non-printable ASCII characters.
+char const *const Nul{"goink\\\000"};
+char const *const Soh{"goink\\\001"};
+char const *const Stx{"goink\\\002"};
+char const *const Etx{"goink\\\003"};
+char const *const Enq{"goink\\\004"};
+char const *const Ack{"goink\\\005"};
+char const *const Bell{"goink\\\afrob"};
+char const *const BackSpace{"goink\\\bfrob"};
+char const *const HorizontalTab{"goink\\\tfrob"};
+char const *const NewLine{"goink\nfrob"};
+char const *const VerticalTab{"goink\\\vfrob"};
+char const *const FormFeed{"goink\\\ffrob"};
+char const *const CarraigeReturn{"goink\\\rfrob"};
+char const *const So{"goink\\\016"};
+char const *const Si{"goink\\\017"};
+char const *const Dle{"goink\\\020"};
+char const *const Dc1{"goink\\\021"};
+char const *const Dc2{"goink\\\022"};
+char const *const Dc3{"goink\\\023"};
+char const *const Dc4{"goink\\\024"};
+char const *const Nak{"goink\\\025"};
+char const *const Syn{"goink\\\026"};
+char const *const Etb{"goink\\\027"};
+char const *const Can{"goink\\\030"};
+char const *const Em{"goink\\\031"};
+char const *const Sub{"goink\\\032"};
+char const *const Esc{"goink\\\033"};
+char const *const Fs{"goink\\\034"};
+char const *const Gs{"goink\\\035"};
+char const *const Rs{"goink\\\036"};
+char const *const Us{"goink\\\037"};
+char const *const HexNonPrintable{"\\\x03"};
+char const *const Delete{"\\\177"};
+
+char const *const TrailingSpace{"A line \\with space. \n"};
+char const *const TrailingNewLine{"A single \\line.\n"};
+char const *const AlreadyRaw{R"(foobie\\bletch)"};
+char const *const UTF8Literal{u8"foobie\\bletch"};
+char const *const UTF8RawLiteral{u8R"(foobie\\bletch)"};
+char16_t const *const UTF16Literal{u"foobie\\bletch"};
+char16_t const *const UTF16RawLiteral{uR"(foobie\\bletch)"};
+char32_t const *const UTF32Literal{U"foobie\\bletch"};
+char32_t const *const UTF32RawLiteral{UR"(foobie\\bletch)"};
+wchar_t const *const WideLiteral{L"foobie\\bletch"};
+wchar_t const *const WideRawLiteral{LR"(foobie\\bletch)"};
+
+char const *const SingleQuote{"goink\'frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
+// CHECK-XFIXES: {{^}}char const *const SingleQuote{R"(goink'frob)"};{{$}}
+
+char const *const DoubleQuote{"goink\"frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const DoubleQuote{R"(goink"frob)"};{{$}}
+
+char const *const QuestionMark{"goink\?frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const QuestionMark{R"(goink?frob)"};{{$}}
+
+char const *const RegEx{"goink\\(one|two\\)\\?.*\\nfrob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const RegEx{R"(goink\(one|two\)\\\?.*\nfrob)"};{{$}}
+
+char const *const Path{"C:\\Program Files\\Vendor\\Application\\Application.exe"};
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const Path{R"(C:\Program Files\Vendor\Application\Application.exe)"};{{$}}
+
+char const *const ContainsSentinel{"who\\ops)\""};
+// CHECK-MESSAGES: :[[@LINE-1]]:36: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const ContainsSentinel{R"lit(who\ops)")lit"};{{$}}
+
+char const *const ContainsDelim{"whoops)\")lit\""};
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const ContainsDelim{R"lit1(whoops)")lit")lit1"};{{$}}
+
+char const *const OctalPrintable{"\100\\"};
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: 

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 47929.
LegalizeAdulthood marked an inline comment as done.
LegalizeAdulthood added a comment.

Add FIXME for macro argument test case.


http://reviews.llvm.org/D16529

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/RawStringLiteralCheck.cpp
  clang-tidy/modernize/RawStringLiteralCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-raw-string-literal.rst
  test/clang-tidy/modernize-raw-string-literal-delimiter.cpp
  test/clang-tidy/modernize-raw-string-literal.cpp

Index: test/clang-tidy/modernize-raw-string-literal.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-raw-string-literal.cpp
@@ -0,0 +1,123 @@
+// RUN: %check_clang_tidy %s modernize-raw-string-literal %t
+
+char const *const BackSlash{"goink\\frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: escaped string literal can be written as a raw string literal [modernize-raw-string-literal]
+// CHECK-FIXES: {{^}}char const *const BackSlash{R"(goink\frob)"};{{$}}
+
+char const *const PlainLiteral("plain literal");
+
+// Non-printable ASCII characters.
+char const *const Nul{"goink\\\000"};
+char const *const Soh{"goink\\\001"};
+char const *const Stx{"goink\\\002"};
+char const *const Etx{"goink\\\003"};
+char const *const Enq{"goink\\\004"};
+char const *const Ack{"goink\\\005"};
+char const *const Bell{"goink\\\afrob"};
+char const *const BackSpace{"goink\\\bfrob"};
+char const *const HorizontalTab{"goink\\\tfrob"};
+char const *const NewLine{"goink\nfrob"};
+char const *const VerticalTab{"goink\\\vfrob"};
+char const *const FormFeed{"goink\\\ffrob"};
+char const *const CarraigeReturn{"goink\\\rfrob"};
+char const *const So{"goink\\\016"};
+char const *const Si{"goink\\\017"};
+char const *const Dle{"goink\\\020"};
+char const *const Dc1{"goink\\\021"};
+char const *const Dc2{"goink\\\022"};
+char const *const Dc3{"goink\\\023"};
+char const *const Dc4{"goink\\\024"};
+char const *const Nak{"goink\\\025"};
+char const *const Syn{"goink\\\026"};
+char const *const Etb{"goink\\\027"};
+char const *const Can{"goink\\\030"};
+char const *const Em{"goink\\\031"};
+char const *const Sub{"goink\\\032"};
+char const *const Esc{"goink\\\033"};
+char const *const Fs{"goink\\\034"};
+char const *const Gs{"goink\\\035"};
+char const *const Rs{"goink\\\036"};
+char const *const Us{"goink\\\037"};
+char const *const HexNonPrintable{"\\\x03"};
+char const *const Delete{"\\\177"};
+
+char const *const TrailingSpace{"A line \\with space. \n"};
+char const *const TrailingNewLine{"A single \\line.\n"};
+char const *const AlreadyRaw{R"(foobie\\bletch)"};
+char const *const UTF8Literal{u8"foobie\\bletch"};
+char const *const UTF8RawLiteral{u8R"(foobie\\bletch)"};
+char16_t const *const UTF16Literal{u"foobie\\bletch"};
+char16_t const *const UTF16RawLiteral{uR"(foobie\\bletch)"};
+char32_t const *const UTF32Literal{U"foobie\\bletch"};
+char32_t const *const UTF32RawLiteral{UR"(foobie\\bletch)"};
+wchar_t const *const WideLiteral{L"foobie\\bletch"};
+wchar_t const *const WideRawLiteral{LR"(foobie\\bletch)"};
+
+char const *const SingleQuote{"goink\'frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
+// CHECK-XFIXES: {{^}}char const *const SingleQuote{R"(goink'frob)"};{{$}}
+
+char const *const DoubleQuote{"goink\"frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const DoubleQuote{R"(goink"frob)"};{{$}}
+
+char const *const QuestionMark{"goink\?frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const QuestionMark{R"(goink?frob)"};{{$}}
+
+char const *const RegEx{"goink\\(one|two\\)\\?.*\\nfrob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const RegEx{R"(goink\(one|two\)\\\?.*\nfrob)"};{{$}}
+
+char const *const Path{"C:\\Program Files\\Vendor\\Application\\Application.exe"};
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const Path{R"(C:\Program Files\Vendor\Application\Application.exe)"};{{$}}
+
+char const *const ContainsSentinel{"who\\ops)\""};
+// CHECK-MESSAGES: :[[@LINE-1]]:36: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const ContainsSentinel{R"lit(who\ops)")lit"};{{$}}
+
+char const *const ContainsDelim{"whoops)\")lit\""};
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const ContainsDelim{R"lit1(whoops)")lit")lit1"};{{$}}
+
+char const *const OctalPrintable{"\100\\"};
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: {{.*}} can be written as a raw string literal
+// 

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:97
@@ +96,3 @@
+}
+
+} // namespace

alexfh wrote:
> > I believe we agree on the following: ...
> 
> Yes.
> 
> > Where we seem to disagree is what algorithm should be used to determine the 
> > delimiter when a delimited raw string literal is required.
> 
> Pretty much so. IMO, we don't need a universal algorithm that will hardly 
> ever go further than the second iteration, and even in this case would 
> produce a result that's likely undesirable (as I said, `R"lit0()lit0"` is 
> not what I would want to see in my code).
> 
> The possibility of this code causing performance issues is probably not that 
> high, but I can imagine a code where this could be sub-optimal.
> 
> > My implementation is the minimum performance impact for the typical case 
> > where the string does not contain )".
> 
> One concern about this part is that it could issue 4 concatenation calls 
> fewer in case of an empty delimiter. This may already be handled well by the 
> `llvm::Twine` though.
> 
> Any code following potentially-problematic patterns might be fine on its own, 
> but it will attract unnecessary attention when reading code. High frequency 
> of such false alarms has non-trivial cost for code readers and makes it 
> harder to find actual problems.
> 
> So please, change the code to avoid these issues. Here's a possible 
> alternative:
> 
>   llvm::Optional asRawStringLiteral(const StringLiteral 
> *Literal) {
> const StringRef Bytes = Literal->getBytes();
> static const StringRef Delimiters[2][] =
>   {{"R\"(", ")\""}, {"R\"lit(", ")lit\""}, {"R\"str(", ")str\""},
>   /* add more different ones to make it unlikely to meet all of these in 
> a single literal in the wild */};
> for (const auto  : Delimiters) {
>   if (Bytes.find(Delim[1]) != StringRef::npos)
> return (Delim[0] + Bytes + Delim[1]).str();
> }
> // Give up gracefully on literals having all our delimiters.
> return llvm::None;
>   }
I just don't see why replacing a general algorithm that always works with a 
bunch of hard-coded special cases is better.

You've reiterated your preference for one over the other, but since it simply a 
matter of preference, I don't see why this should be a requirement.

Working with the hard-coded list of delimiters is no more or less efficient 
than the general algorithm that I implemented, so efficiency is not the concern 
here.


Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:110
@@ +109,3 @@
+  if (const auto *Literal = Result.Nodes.getNodeAs("lit")) {
+if (isMacroExpansionLocation(Result, Literal->getLocStart()))
+  return;

alexfh wrote:
>   if (Literal->getLocStart().isMacroID())
> return;
I don't understand what this code is doing.  `isMacroID` is undocumented, so I 
don't know what it means for this function to return true without reverse 
engineering the implementation.


http://reviews.llvm.org/D16529



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

Re: [PATCH] D16953: Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Squeak?


http://reviews.llvm.org/D16953



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

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Squeak?


http://reviews.llvm.org/D16529



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

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

If someone could review the changes and commit this corrected version for me, 
that would be great.

Patch by Richard Thomson.


http://reviews.llvm.org/D16308



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

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

[Richard via cfe-commits]

In article ,
Daniel Marjamäki  writes:

> > You can get overflow with / and %. Consider:
> >
> > int i = INT_MIN / -1; // similar for %
> 
> you are right, I did not think of that. 
> 
> imho overflow still seems unlikely for / and %.

I thought the whole point was to flag instances where the presence
of a cast indicated a suspicion of intent that wasn't being satisfied?

In other words, overflow may be unlikely but doesn't the presence of
the cast indicate the same suspicious intent?
-- 
"The Direct3D Graphics Pipeline" free book 
 The Computer Graphics Museum 
 The Terminals Wiki 
  Legalize Adulthood! (my blog) 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In article ,

  Aaron Ballman  writes:

> aaron.ballman added a comment.

> 

> This is causing build bot failures:

> 

> http://bb.pgr.jp/builders/cmake-clang-tools-x86_64-linux/builds/23351

>  http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/492

>  http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/812

> 

> The failure is not obvious to me, and I don't have the opportunity to debug

>  locally right now, so I have reverted in r260097.


make check-all passes for me.  I don't see what the problem is in the logs
either.


http://reviews.llvm.org/D16308



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

Re: [PATCH] D16953: Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files

[Richard via cfe-commits]

LegalizeAdulthood marked an inline comment as done.


Comment at: test/clang-tidy/check_clang_tidy.py:152
@@ -68,4 +151,3 @@
 
-  has_check_fixes = input_text.find('CHECK-FIXES') >= 0
-  has_check_messages = input_text.find('CHECK-MESSAGES') >= 0
+  has_check_fixes, has_check_messages = has_checks(input_text)
 

LegalizeAdulthood wrote:
> alexfh wrote:
> > This function doesn't look like a particularly useful one: 5 lines of code 
> > instead of 2 lines and an unclear interface (in which order do the two 
> > bools come?). Please revert this part.
> I initially had it doing the same checks on the header file as well and then 
> it made more sense.  I dug myself into a dead-end on that series of changes, 
> reverted and started over.
> 
> What I settled on here was the assumption that you won't have 
> `CHECK-MESSAGES` or `CHECK-FIXES` in your header file, unless you also had 
> them in the associated source file.  If that assumption is invalid, then I 
> should call this function again for the header and change the tests to be 
> asserting fixes in the source or header, messages in the source or header in 
> various places.
IMO, "5 lines instead of 2 lines" isn't the kind of reasoning that is useful.  
Otherwise we would never do [[ 
https://www.industriallogic.com/xp/refactoring/composeMethod.html | Compose 
Method ]] on a long method since the act of breaking a long method down into a 
bunch of smaller methods necessarily introduces more lines of code.

This script originally was one giant function that did everything.  A bunch of 
things need to be done twice now: one for the source file and once for the 
header file.  Applying Compose Method Extracting results in extracting a group 
of smaller functions/methods.

Now the functions have a bunch of state that needs to be passed around between 
them and the functions might be more clearly expressed as methods on a class, 
but I didn't go that far with it.


http://reviews.llvm.org/D16953



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

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

In article ,
Aaron Ballman  writes:

> aaron.ballman added a comment.
> 
> This is causing build bot failures:
> 
> http://bb.pgr.jp/builders/cmake-clang-tools-x86_64-linux/builds/23351
> http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/492
> http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/812
> 
> The failure is not obvious to me, and I don't have the opportunity to debug
> locally right now, so I have reverted in r260097.

make check-all passes for me.  I don't see what the problem is in the logs
either.
-- 
"The Direct3D Graphics Pipeline" free book 
 The Computer Graphics Museum 
 The Terminals Wiki 
  Legalize Adulthood! (my blog) 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16953: Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 47319.
LegalizeAdulthood added a comment.

Update from review comments


http://reviews.llvm.org/D16953

Files:
  clang-tidy/modernize/RedundantVoidArgCheck.cpp
  test/clang-tidy/check_clang_tidy.py
  test/clang-tidy/modernize-redundant-void-arg.cpp
  test/clang-tidy/modernize-redundant-void-arg.h

Index: test/clang-tidy/modernize-redundant-void-arg.h
===
--- /dev/null
+++ test/clang-tidy/modernize-redundant-void-arg.h
@@ -0,0 +1,8 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H
+#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H
+
+extern int foo(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: redundant void argument list in function declaration [modernize-redundant-void-arg]
+// CHECK-FIXES: {{^}}extern int foo();{{$}}
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H
Index: test/clang-tidy/modernize-redundant-void-arg.cpp
===
--- test/clang-tidy/modernize-redundant-void-arg.cpp
+++ test/clang-tidy/modernize-redundant-void-arg.cpp
@@ -1,4 +1,6 @@
-// RUN: %check_clang_tidy %s modernize-redundant-void-arg %t
+// RUN: %check_clang_tidy --header-filter=modernize-redundant-void-arg.h %s modernize-redundant-void-arg %t
+
+#include "modernize-redundant-void-arg.h"
 
 #define NULL 0
 
Index: test/clang-tidy/check_clang_tidy.py
===
--- test/clang-tidy/check_clang_tidy.py
+++ test/clang-tidy/check_clang_tidy.py
@@ -25,6 +25,7 @@
 """
 
 import argparse
+import os
 import re
 import subprocess
 import sys
@@ -35,16 +36,98 @@
 f.write(text)
 f.truncate()
 
+
+# Remove the contents of the CHECK lines to avoid CHECKs matching on
+# themselves.  We need to keep the comments to preserve line numbers while
+# avoiding empty lines which could potentially trigger formatting-related
+# checks.
+def clean_text(input_text):
+  return re.sub('// *CHECK-[A-Z-]*:[^\r\n]*', '//', input_text)
+
+
+def write_cleaned_header(input_file_path, temp_file_name, header_file_name):
+  src_path = os.path.join(os.path.dirname(input_file_path), header_file_name)
+  fixed_path = os.path.join(os.path.dirname(temp_file_name), header_file_name)
+  with open(src_path, 'r') as input_file:
+cleaned_text = clean_text(input_file.read())
+  cleaned_path = fixed_path + '.orig'
+  write_file(cleaned_path, cleaned_text)
+  write_file(fixed_path, cleaned_text)
+  return cleaned_path, fixed_path, src_path
+
+
+def separate_output(clang_tidy_output, header_file_name, input_file_name):
+  input_file_name = os.path.basename(input_file_name)
+  input_file_output = ''
+  header_file_output = ''
+  input_messages = True
+  for line in clang_tidy_output.splitlines(True):
+if input_messages:
+  if header_file_name in line:
+input_messages = False
+header_file_output += line
+  else:
+input_file_output += line
+else:
+  if input_file_name in line:
+input_messages = True
+input_file_output += line
+  else:
+header_file_output += line
+  return header_file_output, input_file_output
+
+
+def try_run(args):
+  try:
+process_output = \
+  subprocess.check_output(args, stderr=subprocess.STDOUT).decode()
+  except subprocess.CalledProcessError as e:
+print('%s failed:\n%s' % (' '.join(args), e.output.decode()))
+raise
+  return process_output
+
+
+def check_output_for_messages(messages_file, input_file_name):
+  try_run(
+['FileCheck', '-input-file=' + messages_file, input_file_name,
+   '-check-prefix=CHECK-MESSAGES',
+   '-implicit-check-not={{warning|error}}:'])
+
+
+def check_output_for_fixes(temp_file_name, input_file_name):
+  try_run(
+  ['FileCheck', '-input-file=' + temp_file_name, input_file_name,
+   '-check-prefix=CHECK-FIXES', '-strict-whitespace'])
+
+
+def has_checks(input_text):
+  has_check_fixes = input_text.find('CHECK-FIXES') >= 0
+  has_check_messages = input_text.find('CHECK-MESSAGES') >= 0
+  return has_check_fixes, has_check_messages
+
+
+def diff_source_files(original_file_name, temp_file_name):
+  try:
+diff_output = subprocess.check_output(
+  ['diff', '-u', original_file_name, temp_file_name],
+  stderr=subprocess.STDOUT)
+  except subprocess.CalledProcessError as e:
+diff_output = e.output
+  return diff_output
+
+
 def main():
   parser = argparse.ArgumentParser()
   parser.add_argument('-resource-dir')
+  parser.add_argument('--header-filter')
   parser.add_argument('input_file_name')
   parser.add_argument('check_name')
   parser.add_argument('temp_file_name')
 
   args, extra_args = parser.parse_known_args()
 
   resource_dir = args.resource_dir
+  header_filter = args.header_filter
   input_file_name = args.input_file_name
   check_name = args.check_name
   temp_file_name 

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 47414.
LegalizeAdulthood added a comment.

Fixes StringRef problem that crashes tests in release builds only.


http://reviews.llvm.org/D16308

Files:
  clang-tidy/readability/SimplifyBooleanExprCheck.cpp
  clang-tidy/readability/SimplifyBooleanExprCheck.h
  docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
  test/clang-tidy/readability-simplify-bool-expr.cpp

Index: test/clang-tidy/readability-simplify-bool-expr.cpp
===
--- test/clang-tidy/readability-simplify-bool-expr.cpp
+++ test/clang-tidy/readability-simplify-bool-expr.cpp
@@ -690,7 +690,7 @@
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return static_cast(i & 1);{{$}}
+// CHECK-FIXES: {{^}}  return (i & 1) != 0;{{$}}
 
 bool negated_if_implicit_bool_expr(int i) {
   if (i - 1) {
@@ -700,7 +700,7 @@
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return !static_cast(i - 1);{{$}}
+// CHECK-FIXES: {{^}}  return (i - 1) == 0;{{$}}
 
 bool implicit_int(int i) {
   if (i) {
@@ -710,7 +710,7 @@
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return static_cast(i);{{$}}
+// CHECK-FIXES: {{^}}  return i != 0;{{$}}
 
 bool explicit_bool(bool b) {
   if (b) {
@@ -757,7 +757,7 @@
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return static_cast(~i);{{$}}
+// CHECK-FIXES: {{^}}  return ~i != 0;{{$}}
 
 bool logical_or(bool a, bool b) {
   if (a || b) {
@@ -830,7 +830,7 @@
   bool b = i ? true : false;
 }
 // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: {{.*}} in ternary expression result
-// CHECK-FIXES: bool b = static_cast(i);{{$}}
+// CHECK-FIXES: bool b = i != 0;{{$}}
 
 bool non_null_pointer_condition(int *p1) {
   if (p1) {
@@ -895,3 +895,38 @@
 // CHECK-MESSAGES: :[[@LINE-6]]:12: warning: {{.*}} in conditional return
 // CHECK-FIXES: {{^}}  if (b) {
 // CHECK-FIXES: {{^}}#define SOMETHING_WICKED false
+
+bool integer_not_zero(int i) {
+  if (i) {
+return false;
+  } else {
+return true;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}  return i == 0;{{$}}
+
+class A {
+public:
+int m;
+};
+
+bool member_pointer_nullptr(int A::*p) {
+  if (p) {
+return true;
+  } else {
+return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return p != nullptr;{{$}}
+
+bool integer_member_implicit_cast(A *p) {
+  if (p->m) {
+return true;
+  } else {
+return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return p->m != 0;{{$}}
Index: docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
===
--- docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
+++ docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
@@ -35,11 +35,14 @@
   2. Negated applications of ``!`` are eliminated.
   3. Negated applications of comparison operators are changed to use the
  opposite condition.
-  4. Implicit conversions of pointer to ``bool`` are replaced with explicit
- comparisons to ``nullptr``.
+  4. Implicit conversions of pointers, including pointers to members, to
+ ``bool`` are replaced with explicit comparisons to ``nullptr`` in C++11
+  or ``NULL`` in C++98/03.
   5. Implicit casts to ``bool`` are replaced with explicit casts to ``bool``.
   6. Object expressions with ``explicit operator bool`` conversion operators
  are replaced with explicit casts to ``bool``.
+  7. Implicit conversions of integral types to ``bool`` are replaced with
+ explicit comparisons to ``0``.
 
 Examples:
   1. The ternary assignment ``bool b = (i < 0) ? true : false;`` has redundant
@@ -60,11 +63,11 @@
 
  The ternary assignment ``bool b = (i & 1) ? true : false;`` has an
  implicit conversion of ``i & 1`` to ``bool`` and becomes
- ``bool b = static_cast(i & 1);``.
+ ``bool b = (i & 1) != 0;``.
 
   5. The conditional return ``if (i & 1) return true; else return false;`` has
  an implicit conversion of an integer quantity ``i & 1`` to ``bool`` and
- becomes ``return static_cast(i & 1);``
+ becomes ``return (i & 1) != 0;``
 
   6. Given ``struct X { explicit operator bool(); };``, and an instance ``x`` of
  ``struct X``, the conditional return ``if (x) return true; return false;``
Index: clang-tidy/readability/SimplifyBooleanExprCheck.h
===
--- clang-tidy/readability/SimplifyBooleanExprCheck.h
+++ clang-tidy/readability/SimplifyBooleanExprCheck.h
@@ -19,75 +19,8 @@
 /// Looks for boolean expressions involving boolean constants and simplifies
 /// them to 

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: 
clang-tools-extra/trunk/clang-tidy/misc/MisplacedWideningCastCheck.cpp:21-23
@@ +20,5 @@
+void MisplacedWideningCastCheck::registerMatchers(MatchFinder *Finder) {
+  auto Calc = expr(anyOf(binaryOperator(anyOf(
+ hasOperatorName("+"), hasOperatorName("-"),
+ hasOperatorName("*"), hasOperatorName("<<"))),
+ unaryOperator(hasOperatorName("~"))),

Sorry for the late observation, but why doesn't this check for `%` and `/` 
operators?


Repository:
  rL LLVM

http://reviews.llvm.org/D16310



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

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <e1ata3p-0002am...@shell.xmission.com>,
Richard via cfe-commits <cfe-commits@lists.llvm.org> writes:

> make check-all passes for me.  I don't see what the problem is in the logs
> either.

OK, I found the problem.  Once again, StringRef bites me on the behind.

I have updated the review in phabricator with the corrected code.
(The review was closed, so apparently it didn't post to the list
when I updated it on phabricator?)

I surmised that the buildbots do a release build and when I did
check-clang-tools in release, I was able to reproduce the failure.
clang-tidy seg-faults.

It would be nice if we could get the build log output to record that
fact in the log.  I suspect it is the way that clang-tidy is invoked
by the check_clang_tidy.py python script.  Somehow the offending SEGV
message is suppressed.
-- 
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
 The Computer Graphics Museum <http://ComputerGraphicsMuseum.org>
 The Terminals Wiki <http://terminals.classiccmp.org>
  Legalize Adulthood! (my blog) <http://LegalizeAdulthood.wordpress.com>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

I wrote:

> Repeated searching of the string for the literal delimiter could be avoided 
> by changing the routine to search for the delimiter. If present, it could 
> examine the characters following the literal to make the literal more unique 
> and then continue searching from there to look for more occurrences of the 
> extended delimiter. It would proceed incrementally through the rest of the 
> string to create a unique delimiter in a single pass through the string. I 
> think the resulting literals would be even more non-sensical than the current 
> implementation, but it would result in a delimiter obtained by a single pass 
> through the string. It's a significantly more complicated implementation and 
> results in an even weirder delimiter to handle a very edge case.


Unfortunately in this paragraph I used the term "literal" in several places 
where I should have said "delimiter".  I hope that wasn't too confusing.  It 
should have read:

> Repeated searching of the string for the delimiter could be avoided by 
> changing the routine to search for the delimiter. If present, it could 
> examine the characters following the delimiter to make the delimiter more 
> unique and then continue searching from there to look for more occurrences of 
> the extended delimiter. It would proceed incrementally through the rest of 
> the string to create a unique delimiter in a single pass through the string. 
> I think the resulting delimiters would be even more non-sensical than the 
> current implementation, but it would result in a delimiter obtained by a 
> single pass through the string. It's a significantly more complicated 
> implementation and results in an even weirder delimiter to handle a very edge 
> case.



http://reviews.llvm.org/D16529



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

Re: [PATCH] D16953: Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: test/clang-tidy/check_clang_tidy.py:122
@@ -40,2 +121,3 @@
   parser.add_argument('-resource-dir')
+  parser.add_argument('--header-filter')
   parser.add_argument('input_file_name')

alexfh wrote:
> There's no need for a separate argument for this. The script passes all 
> arguments after a `--` to clang-tidy, so you can just append `-- 
> -header-filter=... -- -std=c++11` to the RUN: line in the test.
This argument is the key to making everything work.  It triggers the copying of 
the header, the cleaning of the header of `CHECK-FIXES` and `CHECK-MESSAGES`, 
the diffing of the header, etc.

I originally had it like you suggested by trying to have the test file pass all 
the necessary extra arguments to clang-tidy, but it just isn't enough.  The 
script needs to do a bunch of extra work when headers are involved, so it 
seemed to make the most sense to pass the argument directly to the script so 
that it knows to do this extra work.

In other words, express the intent directly to the script instead of having the 
script infer intent by scraping through extra arguments.

Additionally, this preserves the behavior of eliminating duplicated arguments 
across all clang-tidy tests because the script will assume `-std=c++11` for 
`.cpp` files.


Comment at: test/clang-tidy/check_clang_tidy.py:152
@@ -68,4 +151,3 @@
 
-  has_check_fixes = input_text.find('CHECK-FIXES') >= 0
-  has_check_messages = input_text.find('CHECK-MESSAGES') >= 0
+  has_check_fixes, has_check_messages = has_checks(input_text)
 

alexfh wrote:
> This function doesn't look like a particularly useful one: 5 lines of code 
> instead of 2 lines and an unclear interface (in which order do the two bools 
> come?). Please revert this part.
I initially had it doing the same checks on the header file as well and then it 
made more sense.  I dug myself into a dead-end on that series of changes, 
reverted and started over.

What I settled on here was the assumption that you won't have `CHECK-MESSAGES` 
or `CHECK-FIXES` in your header file, unless you also had them in the 
associated source file.  If that assumption is invalid, then I should call this 
function again for the header and change the tests to be asserting fixes in the 
source or header, messages in the source or header in various places.


http://reviews.llvm.org/D16953



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

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood marked 5 inline comments as done.


Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:96
@@ +95,3 @@
+  std::string Delimiter;
+  for (int Counter = 0; containsDelimiter(Bytes, Delimiter); ++Counter) {
+Delimiter = (Counter == 0) ? "lit" : "lit" + std::to_string(Counter);

alexfh wrote:
> LegalizeAdulthood wrote:
> > alexfh wrote:
> > > Please address my comment above that relates to this code. Specifically, 
> > > I find this generic algorithm unnecessarily inefficient and too rigid, 
> > > i.e. one can't configure a custom set of delimiters that don't follow the 
> > >  pattern. I suggest using a list of pre-concatenated 
> > > pairs of strings (like `R"lit(` and `)lit"`).
> > Raw strings are new and not many people are using them because the don't 
> > have a good way to automatically convert disgusting quoted strings to raw 
> > strings.  So I don't think it is reasonable to draw conclusions by looking 
> > in existing code bases for raw strings.
> > 
> > We're having the same conversation we've had before.  I'm trying to do a 
> > basic check and get things working properly and the review comments are 
> > tending towards a desire for some kind of perfection.
> > 
> > I don't see why we have to make the perfect the enemy of the good.  Nothing 
> > you are suggesting **must** be present now in order for this check to 
> > function properly and reasonably.
> We're looking at this from different perspectives. From my point of view, 
> preventing a potentially wasteful code in ClangTidy checks before it's even 
> committed is much easier than tracking down performance issues when tens of 
> checks run on multiple machines analyzing millions lines of code. So I'm 
> asking to avoid writing code using string allocations and concatenations when 
> there are good alternatives. Apart from possible performance issues, in this 
> case the generic solution you suggest is targets extremely rare cases, while 
> most real-world situations can be handled with a fixed set of delimiters 
> (possibly, configured by the user, while I expect the preferences for the 
> choice of delimiters to be very different in different teams).
I believe we agree on the following:

  - In order to turn a non-raw string literal into a raw string literal I have 
to surround it with `R"(` and `)"`.
  - If the existing string literal contains `)"` already, then surrounding the 
literal with the above will result in a syntax error.  Therefore, every 
potential raw string literal will have to be searched at least once for this 
non-delimited form of raw string literal.
  - `clang-tidy` checks should never introduce syntax errors.

Therefore, I can either not transform the string literal if it contains `)"`, 
or I can come up with some delimited form of raw string literal where the 
delimiter does not appear in the string.  In order to not transform the 
literal, I first have to search for `)"` in order to know that it should not be 
transformed.  Therefore, the search for `)"` must be performed, no matter what 
algorithm is used to determine a delimited or non-delimited raw string literal.

Where we seem to disagree is what algorithm should be used to determine the 
delimiter when a delimited raw string literal is required.

My implementation is the minimum performance impact for the typical case where 
the string does not contain `)"`.

Now let's talk about the cases where searching the string repeatedly for a 
delimiter would be expensive.

First, the string literal will have to contain `)"`.

Second, the string literal would have to be lengthy for the delimiter searching 
to be expensive.  Most of the time lengthy string literals are when someone has 
transformed a text file into a giant string literal.  Such text files include 
newlines and in the latest version of the code, I've decided to skip any string 
literal containing a newline.  It simply results in too many strings being 
converted to raw string literals and obfuscates the newlines.  The one case 
where the embedded newlines makes sense is the case of the text file converted 
into a string literal.

Repeated searching of the string for the literal delimiter could be avoided by 
changing the routine to search for the delimiter.  If present, it could examine 
the characters following the literal to make the literal more unique and then 
continue searching from there to look for more occurrences of the extended 
delimiter.  It would proceed incrementally through the rest of the string to 
create a unique delimiter in a single pass through the string.  I think the 
resulting literals would be even more non-sensical than the current 
implementation, but it would result in a delimiter obtained by a single pass 
through the string.  It's a significantly more complicated implementation and 
results in an even weirder delimiter to handle a very edge case.

If I follow your suggested approach of using a fixed number of string 

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

It's been almost a week.  Is there some reason this hasn't been committed yet?


http://reviews.llvm.org/D16308



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

[PATCH] D16953: Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files

[Richard via cfe-commits]

LegalizeAdulthood created this revision.
LegalizeAdulthood added a reviewer: alexfh.
LegalizeAdulthood added a subscriber: cfe-commits.

Update `check_clang_tidy.py` to handle fixes applied to header files by adding 
`--header-filter` argument that:
- Passes `-header-filter` down to `clang-tidy`
- Copies named header to temporary directory where `clang-tidy` is run
- Performs `FileCheck` checks on the header for `CHECK-MESSAGES`
- Performs `FileCheck` checks on the header for `CHECK-FIXES`

Fixes PR25894

http://reviews.llvm.org/D16953

Files:
  clang-tidy/modernize/RedundantVoidArgCheck.cpp
  test/clang-tidy/check_clang_tidy.py
  test/clang-tidy/modernize-redundant-void-arg.cpp
  test/clang-tidy/modernize-redundant-void-arg.h

Index: test/clang-tidy/modernize-redundant-void-arg.h
===
--- /dev/null
+++ test/clang-tidy/modernize-redundant-void-arg.h
@@ -0,0 +1,8 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H
+#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H
+
+extern int foo(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: redundant void argument list in function declaration [modernize-redundant-void-arg]
+// CHECK-FIXES: {{^}}extern int foo();{{$}}
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H
Index: test/clang-tidy/modernize-redundant-void-arg.cpp
===
--- test/clang-tidy/modernize-redundant-void-arg.cpp
+++ test/clang-tidy/modernize-redundant-void-arg.cpp
@@ -1,4 +1,6 @@
-// RUN: %check_clang_tidy %s modernize-redundant-void-arg %t
+// RUN: %check_clang_tidy --header-filter=modernize-redundant-void-arg.h %s modernize-redundant-void-arg %t
+
+#include "modernize-redundant-void-arg.h"
 
 #define NULL 0
 
Index: test/clang-tidy/check_clang_tidy.py
===
--- test/clang-tidy/check_clang_tidy.py
+++ test/clang-tidy/check_clang_tidy.py
@@ -25,6 +25,7 @@
 """
 
 import argparse
+import os
 import re
 import subprocess
 import sys
@@ -35,16 +36,98 @@
 f.write(text)
 f.truncate()
 
+
+# Remove the contents of the CHECK lines to avoid CHECKs matching on
+# themselves.  We need to keep the comments to preserve line numbers while
+# avoiding empty lines which could potentially trigger formatting-related
+# checks.
+def clean_text(input_text):
+  return re.sub('// *CHECK-[A-Z-]*:[^\r\n]*', '//', input_text)
+
+
+def write_cleaned_header(input_file_path, temp_file_name, header_file_name):
+  src_path = os.path.join(os.path.dirname(input_file_path), header_file_name)
+  fixed_path = os.path.join(os.path.dirname(temp_file_name), header_file_name)
+  with open(src_path, 'r') as input_file:
+cleaned_text = clean_text(input_file.read())
+  cleaned_path = fixed_path + '.orig'
+  write_file(cleaned_path, cleaned_text)
+  write_file(fixed_path, cleaned_text)
+  return cleaned_path, fixed_path, src_path
+
+
+def separate_output(clang_tidy_output, header_file_name, input_file_name):
+  input_file_name = os.path.basename(input_file_name)
+  input_file_output = ''
+  header_file_output = ''
+  input_messages = True
+  for line in clang_tidy_output.splitlines(True):
+if input_messages:
+  if header_file_name in line:
+input_messages = False
+header_file_output += line
+  else:
+input_file_output += line
+else:
+  if input_file_name in line:
+input_messages = True
+input_file_output += line
+  else:
+header_file_output += line
+  return header_file_output, input_file_output
+
+
+def try_run(args):
+  try:
+clang_tidy_output = \
+  subprocess.check_output(args, stderr=subprocess.STDOUT).decode()
+  except subprocess.CalledProcessError as e:
+print('%s failed:\n%s' % (' '.join(args), e.output.decode()))
+raise
+  return clang_tidy_output
+
+
+def check_output_for_messages(messages_file, input_file_name):
+  try_run(
+['FileCheck', '-input-file=' + messages_file, input_file_name,
+   '-check-prefix=CHECK-MESSAGES',
+   '-implicit-check-not={{warning|error}}:'])
+
+
+def check_output_for_fixes(temp_file_name, input_file_name):
+  try_run(
+  ['FileCheck', '-input-file=' + temp_file_name, input_file_name,
+   '-check-prefix=CHECK-FIXES', '-strict-whitespace'])
+
+
+def has_checks(input_text):
+  has_check_fixes = input_text.find('CHECK-FIXES') >= 0
+  has_check_messages = input_text.find('CHECK-MESSAGES') >= 0
+  return has_check_fixes, has_check_messages
+
+
+def diff_source_files(original_file_name, temp_file_name):
+  try:
+diff_output = subprocess.check_output(
+  ['diff', '-u', original_file_name, temp_file_name],
+  stderr=subprocess.STDOUT)
+  except subprocess.CalledProcessError as e:
+diff_output = e.output
+  return diff_output
+
+
 def main():
   parser = argparse.ArgumentParser()
   

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

I agree it needs more testing.

I think also my current approach to newlines is overly aggressive and will 
result in more raw string literals than people would prefer.

It's really the Windows style path separators and regex ones that are not 
controversial transformations.


http://reviews.llvm.org/D16529



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

Re: [PATCH] D16700: [Clang-tidy] Make null pointer literals for fixes configurable for two checks

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: clang-tidy/readability/ImplicitBoolCastCheck.h:32
@@ -30,1 +31,3 @@
+Options.get("NullPointerLiteral",
+Context->getLangOpts().CPlusPlus11 ? "nullptr" : "0")) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;

aaron.ballman wrote:
> I know you are following the pattern used in the code here, but I think this 
> class needs a storeOptions() function as well. See AssertSideEffectCheck as 
> an example.
This will need rebasing on the existing code, which is using "NULL" as the 
pre-C++11 fallback default, not "0".


Comment at: clang-tidy/readability/ImplicitBoolCastCheck.h:36-38
@@ -32,1 +35,5 @@
 
+  StringRef getNullPointerLiteral() const {
+return NullPointerLiteral;
+  }
+

I don't understand why the checks need a public getter for the nullptr literal 
being used.


Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:175
@@ -174,2 +174,3 @@
 
-std::string replacementExpression(const MatchFinder::MatchResult ,
+std::string replacementExpression(SimplifyBooleanExprCheck *Check,
+  const MatchFinder::MatchResult ,

aaron.ballman wrote:
> Check can be a const pointer.
Passing the check in here is overkill.  This helper function isn't going to 
ever be used for multiple checks and the only thing you ever do with the check 
is get the nullptr literal, so just pass in the thing it needs directly.

This will also need to be rebased onto the current code.


Comment at: docs/clang-tidy/checks/readability-simplify-boolean-expr.rst:79-81
@@ -78,1 +78,4 @@
 
+Null pointer literal for fixes could be changed with option
+``NullPointerLiteral``. The default value for C++11 or later is ``nullptr``, 
for
+ C++98/C++03 - ``0``.

The wording here is awkward.  Instead, I suggest:

```
The option ``NullPointerLiteral`` configures the text used for comparisons of 
pointers
against zero to synthesize boolean expressions.  The default value for C++11 or 
later
is ``nullptr``, otherwise the value is ``NULL``.
```

It's subjective, but my experience is that pre C++11 code bases prefer `NULL` 
over `0`.


Repository:
  rL LLVM

http://reviews.llvm.org/D16700



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

Re: [PATCH] D16794: [Clang-tidy] Make readability-simplify-boolean-expr working with included files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D16794#341552, @aaron.ballman wrote:

> Missing a test case for this functionality. The isExpansionInMainFile() was 
> used to silence the diagnostic in macros, but it was pointed out during 
> review that this should be fixed and it seems to have fallen through the 
> cracks. Can you also add tests for macros to ensure the behavior is 
> acceptable there still?
>
> See http://reviews.llvm.org/D8996 for more context.


It hasn't fallen through the cracks, I've been having an offline discussion 
with Alexander Kornienko about how to get automated checks to work for fixits 
applied to headers.


Repository:
  rL LLVM

http://reviews.llvm.org/D16794



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

Re: [PATCH] D16794: [Clang-tidy] Make readability-simplify-boolean-expr working with included files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Again, isn't this already assigned to me in the bug tracker?

In general, my assumption in bug trackers is that if someone has assigned the 
bug to themselves, then they are working on it.


Repository:
  rL LLVM

http://reviews.llvm.org/D16794



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

Re: [PATCH] D16786: [Clang-tidy] Make modernize-redundant-void-arg working with included files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

I am already working on fixing this bug (isn't PR25894 
 already assigned to me?), but 
what's missing from this patch is an automated test that proves it works.

In order to write such an automated test, I believe some enhancements are 
needed to the `check_clang_tidy.py` script before such a test can be written.


Repository:
  rL LLVM

http://reviews.llvm.org/D16786



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

Re: [PATCH] D16259: Add clang-tidy readability-redundant-control-flow check

[Richard via cfe-commits]

In article 

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:366
@@ +365,3 @@
+  binaryOperator(
+  isExpansionInMainFile(), hasOperatorName(OperatorName),
+  hasLHS(allOf(

aaron.ballman wrote:
> Sorry for not noticing this earlier, but since we have two other in-flight 
> patches I reviewed this morning, it caught my attention: we should not be 
> using isExpansionInMainFile, but instead testing the source location of the 
> matches to see if they're in a macro expansion. This is usually done with 
> something like `Result.SourceManager->isMacroBodyExpansion(SomeLoc)`.
> 
> I realize this is existing code and not your problem, but it should be fixed 
> in a follow-on patch (by someone) and include some macro test cases.
There is an open bug on this that I will address.  And while this is existing 
code, I am the author of the original check :).

I also experienced a problem when running this check on some code that did 
something like:

```
#define FOO (par[1] > 4)

// ...
bool f() {
  if (!FOO) {
return true;
  } else {
return false;
  }
}
```

So it needs improvement w.r.t. macros and that is also on my to-do list.

I'm trying to do things in incremental steps and not giant changes.


http://reviews.llvm.org/D16308



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

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

I do not have commit access.  If someone could commit this for me, I would 
appreciate it.

Patch by Richard Thomson.

Thanks.


http://reviews.llvm.org/D16308



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

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.h:21-22
@@ -20,71 +20,4 @@
 /// them to use the appropriate boolean expression directly.
 ///
-/// Examples:
-///
-/// ===  
-/// Initial expression   Result
-/// ---  
-/// `if (b == true)` `if (b)`
-/// `if (b == false)``if (!b)`
-/// `if (b && true)` `if (b)`
-/// `if (b && false)``if (false)`
-/// `if (b || true)` `if (true)`
-/// `if (b || false)``if (b)`
-/// `e ? true : false`   `e`
-/// `e ? false : true`   `!e`
-/// `if (true) t(); else f();`   `t();`
-/// `if (false) t(); else f();`  `f();`
-/// `if (e) return true; else return false;` `return e;`
-/// `if (e) return false; else return true;` `return !e;`
-/// `if (e) b = true; else b = false;`   `b = e;`
-/// `if (e) b = false; else b = true;`   `b = !e;`
-/// `if (e) return true; return false;`  `return e;`
-/// `if (e) return false; return true;`  `return !e;`
-/// ===  
-///
-/// The resulting expression `e` is modified as follows:
-///   1. Unnecessary parentheses around the expression are removed.
-///   2. Negated applications of `!` are eliminated.
-///   3. Negated applications of comparison operators are changed to use the
-///  opposite condition.
-///   4. Implicit conversions of pointer to `bool` are replaced with explicit
-///  comparisons to `nullptr`.
-///   5. Implicit casts to `bool` are replaced with explicit casts to `bool`.
-///   6. Object expressions with `explicit operator bool` conversion operators
-///  are replaced with explicit casts to `bool`.
-///
-/// Examples:
-///   1. The ternary assignment `bool b = (i < 0) ? true : false;` has 
redundant
-///  parentheses and becomes `bool b = i < 0;`.
-///
-///   2. The conditional return `if (!b) return false; return true;` has an
-///  implied double negation and becomes `return b;`.
-///
-///   3. The conditional return `if (i < 0) return false; return true;` becomes
-///  `return i >= 0;`.
-///
-///  The conditional return `if (i != 0) return false; return true;` 
becomes
-///  `return i == 0;`.
-///
-///   4. The conditional return `if (p) return true; return false;` has an
-///  implicit conversion of a pointer to `bool` and becomes
-///  `return p != nullptr;`.
-///
-///  The ternary assignment `bool b = (i & 1) ? true : false;` has an
-///  implicit conversion of `i & 1` to `bool` and becomes
-///  `bool b = static_cast(i & 1);`.
-///
-///   5. The conditional return `if (i & 1) return true; else return false;` 
has
-///  an implicit conversion of an integer quantity `i & 1` to `bool` and
-///  becomes `return static_cast(i & 1);`
-///
-///   6. Given `struct X { explicit operator bool(); };`, and an instance `x` 
of
-///  `struct X`, the conditional return `if (x) return true; return false;`
-///  becomes `return static_cast(x);`
-///
-/// When a conditional boolean return or assignment appears at the end of a
-/// chain of `if`, `else if` statements, the conditional statement is left
-/// unchanged unless the option `ChainedConditionalReturn` or
-/// `ChainedConditionalAssignment`, respectively, is specified as non-zero.
-/// The default value for both options is zero.
-///
+/// For the user-facing documentation see:
+/// 
http://clang.llvm.org/extra/clang-tidy/checks/readability-simplify-boolean-expr.html

aaron.ballman wrote:
> LegalizeAdulthood wrote:
> > aaron.ballman wrote:
> > > LegalizeAdulthood wrote:
> > > > aaron.ballman wrote:
> > > > > I think I can agree with that, but I also think it depends a lot on 
> > > > > what the purpose to the check is. If it's "improve readability 
> > > > > regarding parens" + options that control when to remove or add, that 
> > > > > makes sense to me. Otherwise, I think segregating the checks into one 
> > > > > that removes (plus options) and one that adds (plus options) makes 
> > > > > sense -- even if they perhaps use the same underlying heuristic 
> > > > > engine and are simply surfaced as separate checks.
> > > > This check isn't at all about the readability of parens, so it doesn't 
> > > > make sense for this check to be concerned with it IMO.
> > > Agreed; trying to suss out what the appropriate design for this 
> > > particular check is. I think I've talked myself into "don't touch parens 
> > > here."
> > Currently in this patch, parens are added when the expression compared to 
> > `nullptr` or `0` is a binary operator.
> > 
> > I think 

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.h:21-22
@@ -20,71 +20,4 @@
 /// them to use the appropriate boolean expression directly.
 ///
-/// Examples:
-///
-/// ===  
-/// Initial expression   Result
-/// ---  
-/// `if (b == true)` `if (b)`
-/// `if (b == false)``if (!b)`
-/// `if (b && true)` `if (b)`
-/// `if (b && false)``if (false)`
-/// `if (b || true)` `if (true)`
-/// `if (b || false)``if (b)`
-/// `e ? true : false`   `e`
-/// `e ? false : true`   `!e`
-/// `if (true) t(); else f();`   `t();`
-/// `if (false) t(); else f();`  `f();`
-/// `if (e) return true; else return false;` `return e;`
-/// `if (e) return false; else return true;` `return !e;`
-/// `if (e) b = true; else b = false;`   `b = e;`
-/// `if (e) b = false; else b = true;`   `b = !e;`
-/// `if (e) return true; return false;`  `return e;`
-/// `if (e) return false; return true;`  `return !e;`
-/// ===  
-///
-/// The resulting expression `e` is modified as follows:
-///   1. Unnecessary parentheses around the expression are removed.
-///   2. Negated applications of `!` are eliminated.
-///   3. Negated applications of comparison operators are changed to use the
-///  opposite condition.
-///   4. Implicit conversions of pointer to `bool` are replaced with explicit
-///  comparisons to `nullptr`.
-///   5. Implicit casts to `bool` are replaced with explicit casts to `bool`.
-///   6. Object expressions with `explicit operator bool` conversion operators
-///  are replaced with explicit casts to `bool`.
-///
-/// Examples:
-///   1. The ternary assignment `bool b = (i < 0) ? true : false;` has 
redundant
-///  parentheses and becomes `bool b = i < 0;`.
-///
-///   2. The conditional return `if (!b) return false; return true;` has an
-///  implied double negation and becomes `return b;`.
-///
-///   3. The conditional return `if (i < 0) return false; return true;` becomes
-///  `return i >= 0;`.
-///
-///  The conditional return `if (i != 0) return false; return true;` 
becomes
-///  `return i == 0;`.
-///
-///   4. The conditional return `if (p) return true; return false;` has an
-///  implicit conversion of a pointer to `bool` and becomes
-///  `return p != nullptr;`.
-///
-///  The ternary assignment `bool b = (i & 1) ? true : false;` has an
-///  implicit conversion of `i & 1` to `bool` and becomes
-///  `bool b = static_cast(i & 1);`.
-///
-///   5. The conditional return `if (i & 1) return true; else return false;` 
has
-///  an implicit conversion of an integer quantity `i & 1` to `bool` and
-///  becomes `return static_cast(i & 1);`
-///
-///   6. Given `struct X { explicit operator bool(); };`, and an instance `x` 
of
-///  `struct X`, the conditional return `if (x) return true; return false;`
-///  becomes `return static_cast(x);`
-///
-/// When a conditional boolean return or assignment appears at the end of a
-/// chain of `if`, `else if` statements, the conditional statement is left
-/// unchanged unless the option `ChainedConditionalReturn` or
-/// `ChainedConditionalAssignment`, respectively, is specified as non-zero.
-/// The default value for both options is zero.
-///
+/// For the user-facing documentation see:
+/// 
http://clang.llvm.org/extra/clang-tidy/checks/readability-simplify-boolean-expr.html

aaron.ballman wrote:
> LegalizeAdulthood wrote:
> > aaron.ballman wrote:
> > > I think I can agree with that, but I also think it depends a lot on what 
> > > the purpose to the check is. If it's "improve readability regarding 
> > > parens" + options that control when to remove or add, that makes sense to 
> > > me. Otherwise, I think segregating the checks into one that removes (plus 
> > > options) and one that adds (plus options) makes sense -- even if they 
> > > perhaps use the same underlying heuristic engine and are simply surfaced 
> > > as separate checks.
> > This check isn't at all about the readability of parens, so it doesn't make 
> > sense for this check to be concerned with it IMO.
> Agreed; trying to suss out what the appropriate design for this particular 
> check is. I think I've talked myself into "don't touch parens here."
Currently in this patch, parens are added when the expression compared to 
`nullptr` or `0` is a binary operator.

I think that is the right thing to do here so we get:

```
bool b = (i & 1) == 0;
```

instead of

```
bool b = i & 1 == 0;
```

Re: [PATCH] D16526: Add hasRetValue narrowing matcher for returnStmt

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D16526#334938, @aaron.ballman wrote:

> I'm of two minds on this [...]


I just need a thumbs up/down on this.

Thumbs down, I discard the review.

Thumbs up, you take the patch.

Patch by Richard Thomson.


http://reviews.llvm.org/D16526



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

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.h:21-22
@@ -20,71 +20,4 @@
 /// them to use the appropriate boolean expression directly.
 ///
-/// Examples:
-///
-/// ===  
-/// Initial expression   Result
-/// ---  
-/// `if (b == true)` `if (b)`
-/// `if (b == false)``if (!b)`
-/// `if (b && true)` `if (b)`
-/// `if (b && false)``if (false)`
-/// `if (b || true)` `if (true)`
-/// `if (b || false)``if (b)`
-/// `e ? true : false`   `e`
-/// `e ? false : true`   `!e`
-/// `if (true) t(); else f();`   `t();`
-/// `if (false) t(); else f();`  `f();`
-/// `if (e) return true; else return false;` `return e;`
-/// `if (e) return false; else return true;` `return !e;`
-/// `if (e) b = true; else b = false;`   `b = e;`
-/// `if (e) b = false; else b = true;`   `b = !e;`
-/// `if (e) return true; return false;`  `return e;`
-/// `if (e) return false; return true;`  `return !e;`
-/// ===  
-///
-/// The resulting expression `e` is modified as follows:
-///   1. Unnecessary parentheses around the expression are removed.
-///   2. Negated applications of `!` are eliminated.
-///   3. Negated applications of comparison operators are changed to use the
-///  opposite condition.
-///   4. Implicit conversions of pointer to `bool` are replaced with explicit
-///  comparisons to `nullptr`.
-///   5. Implicit casts to `bool` are replaced with explicit casts to `bool`.
-///   6. Object expressions with `explicit operator bool` conversion operators
-///  are replaced with explicit casts to `bool`.
-///
-/// Examples:
-///   1. The ternary assignment `bool b = (i < 0) ? true : false;` has 
redundant
-///  parentheses and becomes `bool b = i < 0;`.
-///
-///   2. The conditional return `if (!b) return false; return true;` has an
-///  implied double negation and becomes `return b;`.
-///
-///   3. The conditional return `if (i < 0) return false; return true;` becomes
-///  `return i >= 0;`.
-///
-///  The conditional return `if (i != 0) return false; return true;` 
becomes
-///  `return i == 0;`.
-///
-///   4. The conditional return `if (p) return true; return false;` has an
-///  implicit conversion of a pointer to `bool` and becomes
-///  `return p != nullptr;`.
-///
-///  The ternary assignment `bool b = (i & 1) ? true : false;` has an
-///  implicit conversion of `i & 1` to `bool` and becomes
-///  `bool b = static_cast(i & 1);`.
-///
-///   5. The conditional return `if (i & 1) return true; else return false;` 
has
-///  an implicit conversion of an integer quantity `i & 1` to `bool` and
-///  becomes `return static_cast(i & 1);`
-///
-///   6. Given `struct X { explicit operator bool(); };`, and an instance `x` 
of
-///  `struct X`, the conditional return `if (x) return true; return false;`
-///  becomes `return static_cast(x);`
-///
-/// When a conditional boolean return or assignment appears at the end of a
-/// chain of `if`, `else if` statements, the conditional statement is left
-/// unchanged unless the option `ChainedConditionalReturn` or
-/// `ChainedConditionalAssignment`, respectively, is specified as non-zero.
-/// The default value for both options is zero.
-///
+/// For the user-facing documentation see:
+/// 
http://clang.llvm.org/extra/clang-tidy/checks/readability-simplify-boolean-expr.html

aaron.ballman wrote:
> I think I can agree with that, but I also think it depends a lot on what the 
> purpose to the check is. If it's "improve readability regarding parens" + 
> options that control when to remove or add, that makes sense to me. 
> Otherwise, I think segregating the checks into one that removes (plus 
> options) and one that adds (plus options) makes sense -- even if they perhaps 
> use the same underlying heuristic engine and are simply surfaced as separate 
> checks.
This check isn't at all about the readability of parens, so it doesn't make 
sense for this check to be concerned with it IMO.


http://reviews.llvm.org/D16308



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

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:88
@@ +87,3 @@
+}
+
+bool containsDelimiter(StringRef Bytes, const std::string ) {

alexfh wrote:
> aaron.ballman wrote:
> > I think Alex's point is: why not R"('\"?x01)" (removing the need for lit)?
> Exactly, I was only talking about `lit`, not about using the raw string 
> literal.
It was looking a little busy to my eyes with the raw string " and the quoted " 
close together.  It isn't necessary, but IMO improves readability.


Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:96
@@ +95,3 @@
+  std::string Delimiter;
+  for (int Counter = 0; containsDelimiter(Bytes, Delimiter); ++Counter) {
+Delimiter = (Counter == 0) ? "lit" : "lit" + std::to_string(Counter);

alexfh wrote:
> Please address my comment above that relates to this code. Specifically, I 
> find this generic algorithm unnecessarily inefficient and too rigid, i.e. one 
> can't configure a custom set of delimiters that don't follow the 
>  pattern. I suggest using a list of pre-concatenated pairs of 
> strings (like `R"lit(` and `)lit"`).
Raw strings are new and not many people are using them because the don't have a 
good way to automatically convert disgusting quoted strings to raw strings.  So 
I don't think it is reasonable to draw conclusions by looking in existing code 
bases for raw strings.

We're having the same conversation we've had before.  I'm trying to do a basic 
check and get things working properly and the review comments are tending 
towards a desire for some kind of perfection.

I don't see why we have to make the perfect the enemy of the good.  Nothing you 
are suggesting **must** be present now in order for this check to function 
properly and reasonably.


http://reviews.llvm.org/D16529



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

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 46505.
LegalizeAdulthood added a comment.

Update from review comments.
trunk clang-format


http://reviews.llvm.org/D16308

Files:
  clang-tidy/readability/SimplifyBooleanExprCheck.cpp
  clang-tidy/readability/SimplifyBooleanExprCheck.h
  docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
  test/clang-tidy/readability-simplify-bool-expr.cpp

Index: test/clang-tidy/readability-simplify-bool-expr.cpp
===
--- test/clang-tidy/readability-simplify-bool-expr.cpp
+++ test/clang-tidy/readability-simplify-bool-expr.cpp
@@ -690,7 +690,7 @@
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return static_cast(i & 1);{{$}}
+// CHECK-FIXES: {{^}}  return (i & 1) != 0;{{$}}
 
 bool negated_if_implicit_bool_expr(int i) {
   if (i - 1) {
@@ -700,7 +700,7 @@
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return !static_cast(i - 1);{{$}}
+// CHECK-FIXES: {{^}}  return (i - 1) == 0;{{$}}
 
 bool implicit_int(int i) {
   if (i) {
@@ -710,7 +710,7 @@
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return static_cast(i);{{$}}
+// CHECK-FIXES: {{^}}  return i != 0;{{$}}
 
 bool explicit_bool(bool b) {
   if (b) {
@@ -757,7 +757,7 @@
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return static_cast(~i);{{$}}
+// CHECK-FIXES: {{^}}  return ~i != 0;{{$}}
 
 bool logical_or(bool a, bool b) {
   if (a || b) {
@@ -830,7 +830,7 @@
   bool b = i ? true : false;
 }
 // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: {{.*}} in ternary expression result
-// CHECK-FIXES: bool b = static_cast(i);{{$}}
+// CHECK-FIXES: bool b = i != 0;{{$}}
 
 bool non_null_pointer_condition(int *p1) {
   if (p1) {
@@ -895,3 +895,38 @@
 // CHECK-MESSAGES: :[[@LINE-6]]:12: warning: {{.*}} in conditional return
 // CHECK-FIXES: {{^}}  if (b) {
 // CHECK-FIXES: {{^}}#define SOMETHING_WICKED false
+
+bool integer_not_zero(int i) {
+  if (i) {
+return false;
+  } else {
+return true;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}  return i == 0;{{$}}
+
+class A {
+public:
+int m;
+};
+
+bool member_pointer_nullptr(int A::*p) {
+  if (p) {
+return true;
+  } else {
+return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return p != nullptr;{{$}}
+
+bool integer_member_implicit_cast(A *p) {
+  if (p->m) {
+return true;
+  } else {
+return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return p->m != 0;{{$}}
Index: docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
===
--- docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
+++ docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
@@ -35,11 +35,14 @@
   2. Negated applications of ``!`` are eliminated.
   3. Negated applications of comparison operators are changed to use the
  opposite condition.
-  4. Implicit conversions of pointer to ``bool`` are replaced with explicit
- comparisons to ``nullptr``.
+  4. Implicit conversions of pointers, including pointers to members, to
+ ``bool`` are replaced with explicit comparisons to ``nullptr`` in C++11
+  or ``NULL`` in C++98/03.
   5. Implicit casts to ``bool`` are replaced with explicit casts to ``bool``.
   6. Object expressions with ``explicit operator bool`` conversion operators
  are replaced with explicit casts to ``bool``.
+  7. Implicit conversions of integral types to ``bool`` are replaced with
+ explicit comparisons to ``0``.
 
 Examples:
   1. The ternary assignment ``bool b = (i < 0) ? true : false;`` has redundant
@@ -60,11 +63,11 @@
 
  The ternary assignment ``bool b = (i & 1) ? true : false;`` has an
  implicit conversion of ``i & 1`` to ``bool`` and becomes
- ``bool b = static_cast(i & 1);``.
+ ``bool b = (i & 1) != 0;``.
 
   5. The conditional return ``if (i & 1) return true; else return false;`` has
  an implicit conversion of an integer quantity ``i & 1`` to ``bool`` and
- becomes ``return static_cast(i & 1);``
+ becomes ``return (i & 1) != 0;``
 
   6. Given ``struct X { explicit operator bool(); };``, and an instance ``x`` of
  ``struct X``, the conditional return ``if (x) return true; return false;``
Index: clang-tidy/readability/SimplifyBooleanExprCheck.h
===
--- clang-tidy/readability/SimplifyBooleanExprCheck.h
+++ clang-tidy/readability/SimplifyBooleanExprCheck.h
@@ -19,75 +19,8 @@
 /// Looks for boolean expressions involving boolean constants and simplifies
 /// them to use the appropriate 

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 46506.
LegalizeAdulthood added a comment.

Inline Variable


http://reviews.llvm.org/D16308

Files:
  clang-tidy/readability/SimplifyBooleanExprCheck.cpp
  clang-tidy/readability/SimplifyBooleanExprCheck.h
  docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
  test/clang-tidy/readability-simplify-bool-expr.cpp

Index: test/clang-tidy/readability-simplify-bool-expr.cpp
===
--- test/clang-tidy/readability-simplify-bool-expr.cpp
+++ test/clang-tidy/readability-simplify-bool-expr.cpp
@@ -690,7 +690,7 @@
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return static_cast(i & 1);{{$}}
+// CHECK-FIXES: {{^}}  return (i & 1) != 0;{{$}}
 
 bool negated_if_implicit_bool_expr(int i) {
   if (i - 1) {
@@ -700,7 +700,7 @@
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return !static_cast(i - 1);{{$}}
+// CHECK-FIXES: {{^}}  return (i - 1) == 0;{{$}}
 
 bool implicit_int(int i) {
   if (i) {
@@ -710,7 +710,7 @@
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return static_cast(i);{{$}}
+// CHECK-FIXES: {{^}}  return i != 0;{{$}}
 
 bool explicit_bool(bool b) {
   if (b) {
@@ -757,7 +757,7 @@
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return static_cast(~i);{{$}}
+// CHECK-FIXES: {{^}}  return ~i != 0;{{$}}
 
 bool logical_or(bool a, bool b) {
   if (a || b) {
@@ -830,7 +830,7 @@
   bool b = i ? true : false;
 }
 // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: {{.*}} in ternary expression result
-// CHECK-FIXES: bool b = static_cast(i);{{$}}
+// CHECK-FIXES: bool b = i != 0;{{$}}
 
 bool non_null_pointer_condition(int *p1) {
   if (p1) {
@@ -895,3 +895,38 @@
 // CHECK-MESSAGES: :[[@LINE-6]]:12: warning: {{.*}} in conditional return
 // CHECK-FIXES: {{^}}  if (b) {
 // CHECK-FIXES: {{^}}#define SOMETHING_WICKED false
+
+bool integer_not_zero(int i) {
+  if (i) {
+return false;
+  } else {
+return true;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}  return i == 0;{{$}}
+
+class A {
+public:
+int m;
+};
+
+bool member_pointer_nullptr(int A::*p) {
+  if (p) {
+return true;
+  } else {
+return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return p != nullptr;{{$}}
+
+bool integer_member_implicit_cast(A *p) {
+  if (p->m) {
+return true;
+  } else {
+return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return p->m != 0;{{$}}
Index: docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
===
--- docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
+++ docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
@@ -35,11 +35,14 @@
   2. Negated applications of ``!`` are eliminated.
   3. Negated applications of comparison operators are changed to use the
  opposite condition.
-  4. Implicit conversions of pointer to ``bool`` are replaced with explicit
- comparisons to ``nullptr``.
+  4. Implicit conversions of pointers, including pointers to members, to
+ ``bool`` are replaced with explicit comparisons to ``nullptr`` in C++11
+  or ``NULL`` in C++98/03.
   5. Implicit casts to ``bool`` are replaced with explicit casts to ``bool``.
   6. Object expressions with ``explicit operator bool`` conversion operators
  are replaced with explicit casts to ``bool``.
+  7. Implicit conversions of integral types to ``bool`` are replaced with
+ explicit comparisons to ``0``.
 
 Examples:
   1. The ternary assignment ``bool b = (i < 0) ? true : false;`` has redundant
@@ -60,11 +63,11 @@
 
  The ternary assignment ``bool b = (i & 1) ? true : false;`` has an
  implicit conversion of ``i & 1`` to ``bool`` and becomes
- ``bool b = static_cast(i & 1);``.
+ ``bool b = (i & 1) != 0;``.
 
   5. The conditional return ``if (i & 1) return true; else return false;`` has
  an implicit conversion of an integer quantity ``i & 1`` to ``bool`` and
- becomes ``return static_cast(i & 1);``
+ becomes ``return (i & 1) != 0;``
 
   6. Given ``struct X { explicit operator bool(); };``, and an instance ``x`` of
  ``struct X``, the conditional return ``if (x) return true; return false;``
Index: clang-tidy/readability/SimplifyBooleanExprCheck.h
===
--- clang-tidy/readability/SimplifyBooleanExprCheck.h
+++ clang-tidy/readability/SimplifyBooleanExprCheck.h
@@ -19,75 +19,8 @@
 /// Looks for boolean expressions involving boolean constants and simplifies
 /// them to use the appropriate boolean expression directly.
 ///

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 46504.
LegalizeAdulthood added a comment.

Update from comments


http://reviews.llvm.org/D16529

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/RawStringLiteralCheck.cpp
  clang-tidy/modernize/RawStringLiteralCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-raw-string-literal.rst
  test/clang-tidy/modernize-raw-string-literal.cpp

Index: test/clang-tidy/modernize-raw-string-literal.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-raw-string-literal.cpp
@@ -0,0 +1,68 @@
+// RUN: %check_clang_tidy %s modernize-raw-string-literal %t
+
+char const *const BackSlash{"goink\\frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: escaped string literal can be written as a raw string literal [modernize-raw-string-literal]
+// CHECK-FIXES: {{^}}char const *const BackSlash{R"(goink\frob)"};{{$}}
+
+char const *const Bell{"goink\\\afrob"};
+char const *const BackSpace{"goink\\\bfrob"};
+char const *const FormFeed{"goink\\\ffrob"};
+char const *const CarraigeReturn{"goink\\\rfrob"};
+char const *const HorizontalTab{"goink\\\tfrob"};
+char const *const VerticalTab{"goink\\\vfrob"};
+char const *const OctalNonPrintable{"\\\003"};
+char const *const HexNonPrintable{"\\\x03"};
+char const *const Delete{"\\\177"};
+char const *const TrailingSpace{"A line \\with space. \n"};
+char const *const TrailingNewLine{"A single \\line.\n"};
+char const *const AlreadyRaw{R"(foobie\\bletch)"};
+char const *const UTF8Literal{u8"foobie\\bletch"};
+char const *const UTF8RawLiteral{u8R"(foobie\\bletch)"};
+char16_t const *const UTF16Literal{u"foobie\\bletch"};
+char16_t const *const UTF16RawLiteral{uR"(foobie\\bletch)"};
+char32_t const *const UTF32Literal{U"foobie\\bletch"};
+char32_t const *const UTF32RawLiteral{UR"(foobie\\bletch)"};
+wchar_t const *const WideLiteral{L"foobie\\bletch"};
+wchar_t const *const WideRawLiteral{LR"(foobie\\bletch)"};
+
+char const *const NewLine{"goink\nfrob\n"};
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const NewLine{R"(goink{{$}}
+// CHECK-FIXES-NEXT: {{^}}frob{{$}}
+// CHECK-FIXES-NEXT: {{^}})"};{{$}}
+
+char const *const SingleQuote{"goink\'frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
+// CHECK-XFIXES: {{^}}char const *const SingleQuote{R"(goink'frob)"};{{$}}
+
+char const *const DoubleQuote{"goink\"frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const DoubleQuote{R"(goink"frob)"};{{$}}
+
+char const *const QuestionMark{"goink\?frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const QuestionMark{R"(goink?frob)"};{{$}}
+
+char const *const RegEx{"goink\\(one|two\\)\\?.*\\nfrob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const RegEx{R"(goink\(one|two\)\\\?.*\nfrob)"};{{$}}
+
+char const *const Path{"C:\\Program Files\\Vendor\\Application\\Application.exe"};
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const Path{R"(C:\Program Files\Vendor\Application\Application.exe)"};{{$}}
+
+char const *const ContainsSentinel{"who\\ops)\""};
+// CHECK-MESSAGES: :[[@LINE-1]]:36: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const ContainsSentinel{R"lit(who\ops)")lit"};{{$}}
+
+char const *const ContainsDelim{"whoops)\")lit\""};
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const ContainsDelim{R"lit1(whoops)")lit")lit1"};{{$}}
+
+char const *const OctalPrintable{"\100\\"};
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const OctalPrintable{R"(@\)"};{{$}}
+
+char const *const HexPrintable{"\x40\\"};
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const HexPrintable{R"(@\)"};{{$}}
Index: docs/clang-tidy/checks/modernize-raw-string-literal.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-raw-string-literal.rst
@@ -0,0 +1,56 @@
+.. title:: clang-tidy - modernize-raw-string-literal
+
+modernize-raw-string-literal
+
+
+This check selectively replaces string literals containing escaped characters
+with raw string literals.
+
+Example:
+
+.. code-blocK:: c++
+
+  const char *const Quotes{"embedded \"quotes\""};
+  const char *const Paragraph{"Line one.\nLine two.\nLine 

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: docs/clang-tidy/checks/modernize-raw-string-literal.rst:46
@@ +45,3 @@
+editor.  Trailing whitespace is likely to be stripped by editors and other
+tools, changing the meaning of the literal.
+

LegalizeAdulthood wrote:
> It's a habit my fingers learned when they taught me to type on typewriters in 
> high school.
...also, AFAIK, it doesn't matter in terms of the output produced.


http://reviews.llvm.org/D16529



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

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood marked an inline comment as done.


Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:79
@@ +78,3 @@
+  if (Text.find('R') < Text.find('"'))
+return false;
+

Take a look at http://en.cppreference.com/w/cpp/language/string_literal

There can be a prefix before the R, it's not always the first character.


Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:87
@@ +86,3 @@
+  return containsEscapes(Text, R"lit('\"?x01)lit");
+}
+

Without R"()", the \ and " have to be escaped.  Running this check on this code 
would turn this into a raw string literal.


Comment at: docs/clang-tidy/checks/modernize-raw-string-literal.rst:46
@@ +45,3 @@
+editor.  Trailing whitespace is likely to be stripped by editors and other
+tools, changing the meaning of the literal.
+

It's a habit my fingers learned when they taught me to type on typewriters in 
high school.


http://reviews.llvm.org/D16529



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

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 46507.
LegalizeAdulthood added a comment.

Move local function to anonymous namespace


http://reviews.llvm.org/D16308

Files:
  clang-tidy/readability/SimplifyBooleanExprCheck.cpp
  clang-tidy/readability/SimplifyBooleanExprCheck.h
  docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
  test/clang-tidy/readability-simplify-bool-expr.cpp

Index: test/clang-tidy/readability-simplify-bool-expr.cpp
===
--- test/clang-tidy/readability-simplify-bool-expr.cpp
+++ test/clang-tidy/readability-simplify-bool-expr.cpp
@@ -690,7 +690,7 @@
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return static_cast(i & 1);{{$}}
+// CHECK-FIXES: {{^}}  return (i & 1) != 0;{{$}}
 
 bool negated_if_implicit_bool_expr(int i) {
   if (i - 1) {
@@ -700,7 +700,7 @@
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return !static_cast(i - 1);{{$}}
+// CHECK-FIXES: {{^}}  return (i - 1) == 0;{{$}}
 
 bool implicit_int(int i) {
   if (i) {
@@ -710,7 +710,7 @@
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return static_cast(i);{{$}}
+// CHECK-FIXES: {{^}}  return i != 0;{{$}}
 
 bool explicit_bool(bool b) {
   if (b) {
@@ -757,7 +757,7 @@
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return static_cast(~i);{{$}}
+// CHECK-FIXES: {{^}}  return ~i != 0;{{$}}
 
 bool logical_or(bool a, bool b) {
   if (a || b) {
@@ -830,7 +830,7 @@
   bool b = i ? true : false;
 }
 // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: {{.*}} in ternary expression result
-// CHECK-FIXES: bool b = static_cast(i);{{$}}
+// CHECK-FIXES: bool b = i != 0;{{$}}
 
 bool non_null_pointer_condition(int *p1) {
   if (p1) {
@@ -895,3 +895,38 @@
 // CHECK-MESSAGES: :[[@LINE-6]]:12: warning: {{.*}} in conditional return
 // CHECK-FIXES: {{^}}  if (b) {
 // CHECK-FIXES: {{^}}#define SOMETHING_WICKED false
+
+bool integer_not_zero(int i) {
+  if (i) {
+return false;
+  } else {
+return true;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}  return i == 0;{{$}}
+
+class A {
+public:
+int m;
+};
+
+bool member_pointer_nullptr(int A::*p) {
+  if (p) {
+return true;
+  } else {
+return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return p != nullptr;{{$}}
+
+bool integer_member_implicit_cast(A *p) {
+  if (p->m) {
+return true;
+  } else {
+return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return p->m != 0;{{$}}
Index: docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
===
--- docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
+++ docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
@@ -35,11 +35,14 @@
   2. Negated applications of ``!`` are eliminated.
   3. Negated applications of comparison operators are changed to use the
  opposite condition.
-  4. Implicit conversions of pointer to ``bool`` are replaced with explicit
- comparisons to ``nullptr``.
+  4. Implicit conversions of pointers, including pointers to members, to
+ ``bool`` are replaced with explicit comparisons to ``nullptr`` in C++11
+  or ``NULL`` in C++98/03.
   5. Implicit casts to ``bool`` are replaced with explicit casts to ``bool``.
   6. Object expressions with ``explicit operator bool`` conversion operators
  are replaced with explicit casts to ``bool``.
+  7. Implicit conversions of integral types to ``bool`` are replaced with
+ explicit comparisons to ``0``.
 
 Examples:
   1. The ternary assignment ``bool b = (i < 0) ? true : false;`` has redundant
@@ -60,11 +63,11 @@
 
  The ternary assignment ``bool b = (i & 1) ? true : false;`` has an
  implicit conversion of ``i & 1`` to ``bool`` and becomes
- ``bool b = static_cast(i & 1);``.
+ ``bool b = (i & 1) != 0;``.
 
   5. The conditional return ``if (i & 1) return true; else return false;`` has
  an implicit conversion of an integer quantity ``i & 1`` to ``bool`` and
- becomes ``return static_cast(i & 1);``
+ becomes ``return (i & 1) != 0;``
 
   6. Given ``struct X { explicit operator bool(); };``, and an instance ``x`` of
  ``struct X``, the conditional return ``if (x) return true; return false;``
Index: clang-tidy/readability/SimplifyBooleanExprCheck.h
===
--- clang-tidy/readability/SimplifyBooleanExprCheck.h
+++ clang-tidy/readability/SimplifyBooleanExprCheck.h
@@ -19,75 +19,8 @@
 /// Looks for boolean expressions involving boolean constants and simplifies
 /// them to use the appropriate 

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D16529#339192, @bkramer wrote:

> Why are you re-adding all those Makefiles?


Ugh, I didn't even notice they were in there.  It must have errantly slipped in 
from rebasing.


http://reviews.llvm.org/D16529



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

Re: [PATCH] D8149: Extend hasType narrowing matcher for TypedefDecls, add functionProtoType matcher for FunctionProtoType nodes, extend parameterCountIs to FunctionProtoType nodes

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

If someone could commit this for me, that would be great.  I do not have commit 
access.

Patch by Richard Thomson


http://reviews.llvm.org/D8149



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

Re: r259210 - Extend hasType narrowing matcher for TypedefDecls, add functionProtoType matcher for FunctionProtoType nodes, extend parameterCountIs to FunctionProtoType nodes.

[Richard via cfe-commits]

In article ,
Aaron Ballman  writes:

> On Fri, Jan 29, 2016 at 1:28 PM, Hans Wennborg  wrote:
> > This broke tests:
> >
> > http://bb.pgr.jp/builders/cmake-clang-x86_64-linux/builds/44018/steps/test_
clang/logs/Clang-Unit%20%3A%3A%20ASTMatchers__Dynamic__DynamicASTMatchersTests_
_RegistryTest.Errors
> >
> > I've reverted it in r259218.
> 
> Strange, I did not get an email notification about that failure. Thank
> you for reverting to green. Richard, can you look into this? I'm
> happy to re-commit whenever you resolve the issue.

I looked at the error, but I don't understand what to do about it.

There's too much macro-crazy overload junk in ASTMatchers.h for me to
know what the fix is based on the error.
-- 
"The Direct3D Graphics Pipeline" free book 
 The Computer Graphics Museum 
 The Terminals Wiki 
  Legalize Adulthood! (my blog) 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood marked 2 inline comments as done.


Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.h:77
@@ -74,3 +76,3 @@
 ///  implicit conversion of `i & 1` to `bool` and becomes
-///  `bool b = static_cast(i & 1);`.
+///  `bool b = i & 1 != 0;`.
 ///

aaron.ballman wrote:
> Perhaps a different idea regarding parens isn't to add/remove parens in 
> various checks for readability, but instead have two readability checks that 
> do different things (both disabled by default?): (1) remove spurious parens 
> where the presence of the parens has no effect on the order of evaluation of 
> the subexpressions, and (2) add parens where there is an operator precedence 
> difference between the operators of two subexpressions. Both of these checks 
> are at odds with one another (which is why I don't think it makes sense to 
> enable them by default), but both certainly seem like they would be useful.
> 
> Thoughts on the general idea (not trying to sign either of us up for work to 
> implement it)?
I'm of two minds on this, I'm not really coming down hard on either side.  It 
feels like there should be a readability check for removing parenthesis to 
consolidate the heuristics all in one place and that check can in turn be 
configured by parameters.


http://reviews.llvm.org/D16308



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

Re: [PATCH] D8149: Extend hasType narrowing matcher for TypedefDecls, add functionProtoType matcher for FunctionProtoType nodes, extend parameterCountIs to FunctionProtoType nodes

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 46464.
LegalizeAdulthood added a comment.

Fix RegistryTest unit test
make check-all passes


http://reviews.llvm.org/D8149

Files:
  docs/LibASTMatchersReference.html
  include/clang/ASTMatchers/ASTMatchers.h
  include/clang/ASTMatchers/ASTMatchersInternal.h
  lib/ASTMatchers/Dynamic/Registry.cpp
  unittests/ASTMatchers/ASTMatchersTest.cpp
  unittests/ASTMatchers/Dynamic/RegistryTest.cpp

Index: unittests/ASTMatchers/Dynamic/RegistryTest.cpp
===
--- unittests/ASTMatchers/Dynamic/RegistryTest.cpp
+++ unittests/ASTMatchers/Dynamic/RegistryTest.cpp
@@ -421,7 +421,7 @@
constructMatcher("parameterCountIs", 3), Error.get())
   .isNull());
   EXPECT_EQ("Incorrect type for arg 2. (Expected = Matcher) != "
-"(Actual = Matcher)",
+"(Actual = Matcher)",
 Error->toString());
 
   // Bad argument type with variadic.
Index: unittests/ASTMatchers/ASTMatchersTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersTest.cpp
+++ unittests/ASTMatchers/ASTMatchersTest.cpp
@@ -1091,6 +1091,16 @@
   notMatches("class X {}; void y() { X *x; }", varDecl(hasType(ClassX;
 }
 
+TEST(HasType, MatchesTypedefDecl) {
+  EXPECT_TRUE(matches("typedef int X;", typedefDecl(hasType(asString("int");
+  EXPECT_TRUE(matches("typedef const int T;",
+  typedefDecl(hasType(asString("const int");
+  EXPECT_TRUE(notMatches("typedef const int T;",
+ typedefDecl(hasType(asString("int");
+  EXPECT_TRUE(matches("typedef int foo; typedef foo bar;",
+  typedefDecl(hasType(asString("foo")), hasName("bar";
+}
+
 TEST(HasTypeLoc, MatchesDeclaratorDecls) {
   EXPECT_TRUE(matches("int x;",
   varDecl(hasName("x"), hasTypeLoc(loc(asString("int"));
@@ -1563,6 +1573,9 @@
  functionDecl(isVariadic(;
   EXPECT_TRUE(notMatches("void f();", functionDecl(isVariadic(;
   EXPECT_TRUE(notMatchesC("void f();", functionDecl(isVariadic(;
+  EXPECT_TRUE(matches("void f(...);", functionDecl(parameterCountIs(0;
+  EXPECT_TRUE(matchesC("void f();", functionDecl(parameterCountIs(0;
+  EXPECT_TRUE(matches("void f(int, ...);", functionDecl(parameterCountIs(1;
 }
 
 TEST(FunctionTemplate, MatchesFunctionTemplateDeclarations) {
@@ -1719,6 +1732,7 @@
   EXPECT_TRUE(matches("class X { void f(int i) {} };", Function1Arg));
   EXPECT_TRUE(notMatches("void f() {}", Function1Arg));
   EXPECT_TRUE(notMatches("void f(int i, int j, int k) {}", Function1Arg));
+  EXPECT_TRUE(matches("void f(int i, ...) {};", Function1Arg));
 }
 
 TEST(Matcher, References) {
@@ -4447,6 +4461,15 @@
   EXPECT_TRUE(matches("void f(int i) {}", functionType()));
 }
 
+TEST(TypeMatching, MatchesFunctionProtoTypes) {
+  EXPECT_TRUE(matches("int (*f)(int);", functionProtoType()));
+  EXPECT_TRUE(matches("void f(int i);", functionProtoType()));
+  EXPECT_TRUE(matches("void f();", functionProtoType(parameterCountIs(0;
+  EXPECT_TRUE(notMatchesC("void f();", functionProtoType()));
+  EXPECT_TRUE(
+  matchesC("void f(void);", functionProtoType(parameterCountIs(0;
+}
+
 TEST(TypeMatching, MatchesParenType) {
   EXPECT_TRUE(
   matches("int (*array)[4];", varDecl(hasType(pointsTo(parenType());
@@ -5148,7 +5171,8 @@
   namespaceDecl(isInline(), hasName("m";
 }
 
-// FIXME: Figure out how to specify paths so the following tests pass on Windows.
+// FIXME: Figure out how to specify paths so the following tests pass on
+// Windows.
 #ifndef LLVM_ON_WIN32
 
 TEST(Matcher, IsExpansionInMainFileMatcher) {
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -182,6 +182,7 @@
   REGISTER_MATCHER(forStmt);
   REGISTER_MATCHER(friendDecl);
   REGISTER_MATCHER(functionDecl);
+  REGISTER_MATCHER(functionProtoType);
   REGISTER_MATCHER(functionTemplateDecl);
   REGISTER_MATCHER(functionType);
   REGISTER_MATCHER(gotoStmt);
Index: include/clang/ASTMatchers/ASTMatchersInternal.h
===
--- include/clang/ASTMatchers/ASTMatchersInternal.h
+++ include/clang/ASTMatchers/ASTMatchersInternal.h
@@ -60,6 +60,17 @@
 
 namespace internal {
 
+/// \brief Unifies obtaining the underlying type of a regular node through
+/// `getType` and a TypedefNameDecl node through `getUnderlyingType`.
+template 
+inline QualType getUnderlyingType(const NodeType ) {
+  return Node.getType();
+}
+
+template <> inline QualType getUnderlyingType(const TypedefDecl ) {
+  return Node.getUnderlyingType();
+}
+
 /// \brief Internal version of BoundNodes. Holds all the bound nodes.
 class BoundNodesMap {