This is an archive of the discontinued LLVM Phabricator instance.

Differential D54311

Add a test checking clang-tidy can find libc++ on Mac
ClosedPublic

Authored by ilya-biryukov on Nov 9 2018, 6:51 AM.

Details

Reviewers
sammccall
arphaman
EricWF
Commits
rGac7c4f1db363: Add a test checking clang-tidy can find libc++ on Mac
rL346653: Add a test checking clang-tidy can find libc++ on Mac
rCTE346653: Add a test checking clang-tidy can find libc++ on Mac

Diff Detail

Event Timeline

ilya-biryukov created this revision.Nov 9 2018, 6:51 AM
Herald added a reviewer: EricWF. · View Herald TranscriptNov 9 2018, 6:51 AM
Herald added a subscriber: christof. · View Herald Transcript
Harbormaster completed remote builds in B24776: Diff 173321.Nov 9 2018, 6:51 AM
sammccall added inline comments.Nov 12 2018, 1:28 AM
test/clang-tidy/clang-tidy-mac-libcxx.cpp
12

should this be "command":"clang++"... with clang++ on the PATH?

14

This should check a diagnostic rather than rely on the error code, I think

ilya-biryukov updated this revision to Diff 173630.Nov 12 2018, 1:42 AM
  • Updated the test
Harbormaster completed remote builds in B24877: Diff 173630.Nov 12 2018, 1:42 AM
ilya-biryukov marked an inline comment as done.Nov 12 2018, 1:42 AM
ilya-biryukov added inline comments.
test/clang-tidy/clang-tidy-mac-libcxx.cpp
12

Both the relative and the absolute path should work, but cmake produces absolute paths. Hence the test.

14

Done. We rely on the absence of a diagnostic (the unresolved error for vector), error-code seemed like a simpler way to do this.

sammccall accepted this revision.Nov 12 2018, 1:47 AM
sammccall added inline comments.
test/clang-tidy/clang-tidy-mac-libcxx.cpp
12

Is there a test for the other case? (In the clang repo rather than here is fine)

14

I think emitting a diagnostic would make this a much more robust test, but up to you.

This revision is now accepted and ready to land.Nov 12 2018, 1:47 AM
ilya-biryukov marked an inline comment as done.Nov 12 2018, 1:53 AM
ilya-biryukov added inline comments.
test/clang-tidy/clang-tidy-mac-libcxx.cpp
12

No, will add one in the clang repo.

ilya-biryukov updated this revision to Diff 173635.Nov 12 2018, 2:00 AM
ilya-biryukov marked an inline comment as done.
  • Check for a diagnostic inside the found library
ilya-biryukov updated this revision to Diff 173659.Nov 12 2018, 5:46 AM
  • Check with a clang-tidy warning instead of a clang error
Harbormaster completed remote builds in B24887: Diff 173659.Nov 12 2018, 5:46 AM
Closed by commit rCTE346653: Add a test checking clang-tidy can find libc++ on Mac (authored by ibiryukov). · Explain WhyNov 12 2018, 5:58 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
test/
clang-tidy/
Inputs/
mock-libcxx/
include/
c++/
v1/
2 lines
17 lines

Diff 173664

test/clang-tidy/Inputs/mock-libcxx/include/c++/v1/mock_vector

class vector {};
typedef vector* vector_ptr;

test/clang-tidy/clang-tidy-mac-libcxx.cpp

// Clang on MacOS can find libc++ living beside the installed compiler.
// This test makes sure clang-tidy emulates this properly.
//
// RUN: rm -rf %t
// RUN: mkdir %t
//
// Install the mock libc++ (simulates the libc++ directory structure).
// RUN: cp -r %S/Inputs/mock-libcxx %t/
//
// Pretend clang is installed beside the mock library that we provided.
// RUN: echo '[{"directory":"%t","command":"%t/mock-libcxx/bin/clang++ -stdlib=libc++ -target x86_64-apple-darwin -c test.cpp","file":"test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
// RUN: cp "%s" "%t/test.cpp"
sammccallUnsubmitted
Not Done
ReplyInline Actions

should this be "command":"clang++"... with clang++ on the PATH?

sammccall: should this be `"command":"clang++"...` with clang++ on the PATH?
ilya-biryukovAuthorUnsubmitted
Not Done
ReplyInline Actions

Both the relative and the absolute path should work, but cmake produces absolute paths. Hence the test.

ilya-biryukov: Both the relative and the absolute path should work, but cmake produces absolute paths. Hence…
sammccallUnsubmitted
Not Done
ReplyInline Actions

Is there a test for the other case? (In the clang repo rather than here is fine)

sammccall: Is there a test for the other case? (In the clang repo rather than here is fine)
ilya-biryukovAuthorUnsubmitted
Not Done
ReplyInline Actions

No, will add one in the clang repo.

ilya-biryukov: No, will add one in the clang repo.
// RUN: clang-tidy -header-filter='.*' -system-headers -checks='-*,modernize-use-using' "%t/test.cpp" | FileCheck %s
// CHECK: mock_vector:{{[0-9]+}}:{{[0-9]+}}: warning: use 'using' instead of 'typedef'
sammccallUnsubmitted
Done
ReplyInline Actions

This should check a diagnostic rather than rely on the error code, I think

sammccall: This should check a diagnostic rather than rely on the error code, I think
ilya-biryukovAuthorUnsubmitted
Not Done
ReplyInline Actions

Done. We rely on the absence of a diagnostic (the unresolved error for vector), error-code seemed like a simpler way to do this.

ilya-biryukov: Done. We rely on the absence of a diagnostic (the unresolved error for vector), error-code…
sammccallUnsubmitted
Done
ReplyInline Actions

I think emitting a diagnostic would make this a much more robust test, but up to you.

sammccall: I think emitting a diagnostic would make this a much more robust test, but up to you.
#include <mock_vector>
typedef vector* vec_ptr;