[clang-tools-extra] r315060 - Renaming a test to start with the name of the check based on post-commit review feedback; NFC.

Thu Oct 12 03:46:56 PDT 2017

On Thu, Oct 12, 2017 at 11:12 AM, Jonas Toth <jonas.toth at gmail.com> wrote:

> Hi,
>
> I am not sure if could follow correctly, but the test for nested
> namespaces comments does not work correctly and is therefore currently
> disabled by fileextension?
>

No, the removal of the file extension was unintentional (likely
misunderstanding of my post-commit review comment).

>
> When running the check from trunk i get the following issues:
>
> $ clang-check google-readability-namespace-comments-cxx17.cpp --
> -std=c++17
> > error: invalid value 'c++17' in '-std=c++17'
>

I suppose, your clang-check is just old. Clang supports -std=c++17 these
days.

>
> In the clang-tidy check '-std=c++17' is used. I don't know if the c++17 -
> flag is introduced in clang yet, but maybe this would be one issue.
>
> Running clang-tidy from command line does not produce output (added
> fileextension for now):
>
> $ clang-tidy -checks=-*,google-readability-namespace-comments
> google-readability-namespace-comments-cxx17.cpp -- -std=c++1z
> $ clang-tidy -checks=-*,google-readability-namespace-comments
> google-readability-namespace-comments-cxx17.cpp -- -- -std=c++1z
> > Error while processing /home/jonas/opt/llvm/tools/cla
> ng/tools/extra/test/clang-tidy/google-readability-namespace-
> comments-cxx17.cpp.
>
> Adding a 'std::cout << "Test if run" << std::endl' at the beginning of the
> `check` - Method works and generates output:
>
> $ clang-tidy -checks=-*,google-readability-namespace-comments
> google-readability-namespace-comments-cxx17.cpp -- -std=c++1z
> Test if run
> Test if run
> Test if run
>
> So I think there is a regression in the Testingcode not catching the
> nested namespaces. Did it work before and what could have changed?
> If it did work and the clang-tidy code is the same this could be a
> regression somewhere else.
>

The problem was that the namespace was shorter than the default
ShortNamespaceLines option value, so the check didn't trigger. Fixed
in r315574 (but would be nice, if this kind of stuff was detected before
commit).

However, this is not the end of problems with this check. It started
causing assertion failures on some of our code after the recent changes
(r315057). I'll post an update on that revision once I have more details.

>
> All the Best, Jonas
>
>
> Am 11.10.2017 um 21:36 schrieb Aaron Ballman:
>
>> On Wed, Oct 11, 2017 at 3:29 PM, Alexander Kornienko <alexfh at google.com>
>> wrote:
>>
>>> On Fri, Oct 6, 2017 at 3:27 PM, Aaron Ballman via cfe-commits
>>> <cfe-commits at lists.llvm.org> wrote:
>>>
>>>> Author: aaronballman
>>>> Date: Fri Oct  6 06:27:59 2017
>>>> New Revision: 315060
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=315060&view=rev
>>>> Log:
>>>> Renaming a test to start with the name of the check based on post-commit
>>>> review feedback; NFC.
>>>>
>>>> Added:
>>>>
>>>> clang-tools-extra/trunk/test/clang-tidy/google-readability-n
>>>> amespace-comments-cxx17
>>>>
>>>
>>> Sorry for not being clear. I didn't mean the `.cpp` extension should be
>>> removed. This effectively disables the test, since lit only runs tests in
>>> files with certain extensions (under clang-tools-extra/test these are
>>> '.c',
>>> '.cpp', '.hpp', '.m', '.mm', '.cu', '.ll', '.cl', '.s', '.modularize',
>>> '.module-map-checker', '.test').
>>>
>> That's entirely my fault -- I should have recognized that. Sorry for
>> the trouble!
>>
>> I've just renamed the file to *.cpp and the test fails for me:
>>>
>>> [0/1] Running the Clang extra tools' regression tests
>>> FAIL: Clang Tools ::
>>> clang-tidy/google-readability-namespace-comments-cxx17.cpp (102 of 674)
>>> ******************** TEST 'Clang Tools ::
>>> clang-tidy/google-readability-namespace-comments-cxx17.cpp' FAILED
>>> ********************
>>> Script:
>>> --
>>> /usr/bin/python2.7
>>> /src/tools/clang/tools/extra/test/../test/clang-tidy/check_clang_tidy.py
>>> /src/tools/clang/tools/extra/test/clang-tidy/google-readabil
>>> ity-namespace-comments-cxx17.cpp
>>> google-readability-namespace-comments
>>> /build/tools/clang/tools/extra/test/clang-tidy/Output/google
>>> -readability-namespace-comments-cxx17.cpp.tmp
>>> -- -- -std=c++17
>>> --
>>> Exit Code: 1
>>>
>>> Command Output (stdout):
>>> --
>>> Running ['clang-tidy',
>>> '/build/tools/clang/tools/extra/test/clang-tidy/Output/googl
>>> e-readability-namespace-comments-cxx17.cpp.tmp.cpp',
>>> '-fix', '--checks=-*,google-readability-namespace-comments', '--',
>>> '-std=c++17', '-nostdinc++']...
>>> ------------------------ clang-tidy output -----------------------
>>>
>>> ------------------------------------------------------------------
>>> ------------------------------ Fixes -----------------------------
>>>
>>> ------------------------------------------------------------------
>>> FileCheck failed:
>>> /src/tools/clang/tools/extra/test/clang-tidy/google-readabil
>>> ity-namespace-comments-cxx17.cpp:13:17:
>>> error: expected string not found in input
>>> // CHECK-FIXES: }  // namespace n3
>>>                  ^
>>> /build/tools/clang/tools/extra/test/clang-tidy/Output/google
>>> -readability-namespace-comments-cxx17.cpp.tmp.cpp:1:1:
>>> note: scanning from here
>>> // RUN: %check_clang_tidy %s google-readability-namespace-comments %t
>>> -- --
>>> -std=c++17
>>> ^
>>> /build/tools/clang/tools/extra/test/clang-tidy/Output/google
>>> -readability-namespace-comments-cxx17.cpp.tmp.cpp:5:7:
>>> note: possible intended match here
>>>    // So that namespace is not empty.
>>>        ^
>>>
>>>
>>> --
>>> Command Output (stderr):
>>> --
>>> Traceback (most recent call last):
>>>    File
>>> "/src/tools/clang/tools/extra/test/../test/clang-tidy/check_
>>> clang_tidy.py",
>>> line 140, in <module>
>>>      main()
>>>    File
>>> "/src/tools/clang/tools/extra/test/../test/clang-tidy/check_
>>> clang_tidy.py",
>>> line 121, in main
>>>      stderr=subprocess.STDOUT)
>>>    File "/usr/lib/python2.7/subprocess.py", line 573, in check_output
>>>      raise CalledProcessError(retcode, cmd, output=output)
>>> subprocess.CalledProcessError: Command '['FileCheck',
>>> '-input-file=/build/tools/clang/tools/extra/test/clang-tidy/
>>> Output/google-readability-namespace-comments-cxx17.cpp.tmp.cpp',
>>> '/src/tools/clang/tools/extra/test/clang-tidy/google-readabi
>>> lity-namespace-comments-cxx17.cpp',
>>> '-check-prefix=CHECK-FIXES', '-strict-whitespace']' returned non-zero
>>> exit
>>> status 1
>>>
>>> --
>>>
>>> ********************
>>> Testing Time: 13.07s
>>> ********************
>>> Failing Tests (1):
>>>      Clang Tools ::
>>> clang-tidy/google-readability-namespace-comments-cxx17.cpp
>>>
>>>    Expected Passes    : 673
>>>    Unexpected Failures: 1
>>> FAILED: tools/clang/tools/extra/test/CMakeFiles/check-clang-tools
>>>
>>>
>>> Did you experience anything similar? Any ideas?
>>>
>> I now get the same behavior that you're seeing. I'm not certain what's
>> going on there (and don't have the time to look into it at the
>> moment), but perhaps Jonas has ideas.
>>
>> ~Aaron
>>
>>        - copied unchanged from r315059,
>>>> clang-tools-extra/trunk/test/clang-tidy/google-readability-n
>>>> ested-namespace-comments.cpp
>>>> Removed:
>>>>
>>>> clang-tools-extra/trunk/test/clang-tidy/google-readability-n
>>>> ested-namespace-comments.cpp
>>>>
>>>> Removed:
>>>> clang-tools-extra/trunk/test/clang-tidy/google-readability-n
>>>> ested-namespace-comments.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/
>>>> test/clang-tidy/google-readability-nested-namespace-comments
>>>> .cpp?rev=315059&view=auto
>>>>
>>>> ============================================================
>>>> ==================
>>>> ---
>>>> clang-tools-extra/trunk/test/clang-tidy/google-readability-n
>>>> ested-namespace-comments.cpp
>>>> (original)
>>>> +++
>>>> clang-tools-extra/trunk/test/clang-tidy/google-readability-n
>>>> ested-namespace-comments.cpp
>>>> (removed)
>>>> @@ -1,15 +0,0 @@
>>>> -// RUN: %check_clang_tidy %s google-readability-namespace-comments %t
>>>> --
>>>> -- -std=c++17
>>>> -
>>>> -namespace n1::n2 {
>>>> -namespace n3 {
>>>> -  // So that namespace is not empty.
>>>> -  void f();
>>>> -
>>>> -// CHECK-MESSAGES: :[[@LINE+4]]:2: warning: namespace 'n3' not
>>>> terminated
>>>> with
>>>> -// CHECK-MESSAGES: :[[@LINE-7]]:11: note: namespace 'n3' starts here
>>>> -// CHECK-MESSAGES: :[[@LINE+2]]:3: warning: namespace 'n1::n2' not
>>>> terminated with a closing comment [google-readability-namespace-
>>>> comments]
>>>> -// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'n1::n2' starts
>>>> here
>>>> -}}
>>>> -// CHECK-FIXES: }  // namespace n3
>>>> -// CHECK-FIXES: }  // namespace n1::n2
>>>> -
>>>>
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>
>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171012/2d495aa4/attachment-0001.html>