>>! In D5877#18, @danalbert wrote:
> Mega patches make me sad. I see 5 distinct changes here:
>
> 1. Add missing testit features.
> 2. scan-build support.
> 3. clang-tidy support.
> 4. Coverage support.
> 5. REQUIRES-XFAIL/UNSUPPORTED-XFAIL.
>
> Please split up this change into separate patches. Voltron-esque patches are
> hard to review, since we have to context switch so many times, and they're
> less useful when looking at git history because it isn't always immediately
> obvious which of the many things a given change was for.
Sorry about that. I'll split them up. I was hoping to get away with it this
once. It's hard to maintain and merge 5 separate patches that exist in such a
small space.
================
Comment at: cmake/Modules/LibcxxCodeCoverage.cmake:2
@@ +1,3 @@
+
+find_program(LIBCXX_GCOV gcov)
+if (NOT LIBCXX_GCOV)
----------------
danalbert wrote:
> This looks like a leftover.
lcov depends on gcov. I thought we should check if gcov is found and issue a
hard error otherwise.
================
Comment at: cmake/Modules/LibcxxCodeCoverage.cmake:10
@@ +9,3 @@
+
+set(LIBCXX_CXX_COVERAGE_FLAGS "-g -O0 --coverage -fprofile-arcs
-ftest-coverage")
+
----------------
danalbert wrote:
> iirc --coverage implies -fprofile-arcs and -ftest-coverage.
Yep. My mistake. Thanks.
================
Comment at: cmake/Modules/LibcxxCodeCoverage.cmake:20
@@ +19,3 @@
+
+ file(MAKE_DIRECTORY ${_coverage_dir})
+
----------------
danalbert wrote:
> MAKE_DIRECTORY?
It creates a directory where all of the coverage files will live.
================
Comment at: cmake/Modules/LibcxxCodeCoverage.cmake:22
@@ +21,3 @@
+
+ add_custom_command(TARGET cxx
+ PRE_BUILD
----------------
danalbert wrote:
> Don't we already have a cxx target?
Yep. This adds a command that executes before the cxx target.
================
Comment at: cmake/Modules/LibcxxCodeCoverage.cmake:37
@@ +36,3 @@
+ COMMAND ${CMAKE_COMMAND} -E remove_directory ${_coverage_dir}/test
+ COMMAND find libcxx_coverage/ -type d -exec chmod 750 ${_CHMOD_END}
+ COMMAND find libcxx_coverage/ -type f -exec chmod 640 ${_CHMOD_END}
----------------
danalbert wrote:
> Why?
In case the resulting HTML is hosted on a web server this minimizes the
permissions.
================
Comment at: test/lit.cfg:47
@@ -37,19 +46,3 @@
self.exec_env = dict(exec_env)
-
- def execute_command(self, command, in_dir=None):
- kwargs = {
- 'stdin' :subprocess.PIPE,
- 'stdout':subprocess.PIPE,
- 'stderr':subprocess.PIPE,
- }
- if in_dir:
- kwargs['cwd'] = in_dir
- p = subprocess.Popen(command, **kwargs)
- out,err = p.communicate()
- exitCode = p.wait()
-
- # Detect Ctrl-C in subprocess.
- if exitCode == -signal.SIGINT:
- raise KeyboardInterrupt
-
- return out, err, exitCode
+ self.test_root = str(test_root)
+ self.generate_coverage = generate_coverage
----------------
danalbert wrote:
> What was it before if not a string?
I was being dumb.
================
Comment at: test/lit.cfg:49
@@ +48,3 @@
+ self.generate_coverage = generate_coverage
+ self.coverage_flags = list(coverage_flags)
+ self.coverage_root = str(coverage_root)
----------------
danalbert wrote:
> I don't think this does what you think it does.
>
> You're probably looking for coverage_flags.split(' ')
coverage_flags should already be a list. This performs a copy of the list so we
have our own.
================
Comment at: test/lit.cfg:51
@@ +50,3 @@
+ self.coverage_root = str(coverage_root)
+ self.use_scan_build = use_scan_build
+ self.scan_build_output = scan_build_output
----------------
danalbert wrote:
> Is this actually interesting to run on the tests? I would think this is the
> kind of thing we should be running on libc++ itself.
Yes. The tests instantiate the templates. Without the instantiations the static
analyzer has nothing to do.
================
Comment at: test/lit.cfg:57
@@ +56,3 @@
+
+ def execute_command_and_report(self, command, cwd=None, env=None,
compile_cmd=None):
+ exec_env = dict(os.environ)
----------------
danalbert wrote:
> Diff fail :(
>
> I think the new name is actually a better match for the old behavior (since
> this doesn't actually do the reporting now). I'd change the name back.
>
> Hard to see what else changed... (should be more clear if the name changes
> back).
Ok. Will do.
================
Comment at: test/lit.cfg:105
@@ -72,10 +104,3 @@
for ln in f:
- if 'XFAIL:' in ln:
- items = ln[ln.index('XFAIL:') + 6:].split(',')
- test.xfails.extend([s.strip() for s in items])
- elif 'REQUIRES:' in ln:
- items = ln[ln.index('REQUIRES:') + 9:].split(',')
- requires.extend([s.strip() for s in items])
- elif 'UNSUPPORTED:' in ln:
- items = ln[ln.index('UNSUPPORTED:') + 12:].split(',')
- unsupported.extend([s.strip() for s in items])
+ if self._handle_metadata_line(ln, 'REQUIRES-XFAIL:',
requires_xfail):
+ pass
----------------
danalbert wrote:
> Do we actually need these? It looks like it's just complicating things.
I'll pull these out for now, but the change is something I would like. It gives
the ability to file the test as unsupported while ensuring it fails. It would
be helpful for ignoring the std::tuple tests in c++03 mode.
================
Comment at: test/lit.cfg:200
@@ +199,3 @@
+ # If we are generating code coverage data run the compile command from
+ # the directory the .gcno and .gcda files should be put in.
+ build_cwd = None
----------------
danalbert wrote:
> Hmm. I thought the driver would dump the gcno in the same directory as the
> output file (ditto for the gcd). What is this for?
The driver dumps the gcno file in the cwd used to invoke the compiler. Since
the gcno will be named `<test-name>.gcno` and not the unique executable output
name we need to mirror the test directory structure to ensure that gcno files
for each test don't conflict.
================
Comment at: test/lit.cfg:306
@@ -219,3 +305,3 @@
return None
- if conf.lower() in ('1', 'true'):
+ if conf.lower() in ('1', 'true', 'on'):
return True
----------------
danalbert wrote:
> Why do we need on/off? Doesn't that just mean cmake is missing a
> pythonize_bool?
This input could also come from the command line. I'm not opposed to only using
one name for `true` and `false` but this gives flexibility.
================
Comment at: test/lit.cfg:313
@@ +312,3 @@
+
+ def get_lit_switch(self, name):
+ s = self.get_lit_conf(name)
----------------
danalbert wrote:
> Why do we have two of these?
Different semantics. I'm not a fan of the `get_lit_bool` semantics.
`get_lit_bool`:
* None -> None
* "" -> False
`get_lit_switch`:
* None -> False
* "" -> True
get_lit_switch allows the no-argument case to return True, and always returns a
bool.
================
Comment at: test/lit.cfg:496
@@ +495,3 @@
+ self.lit_config.warning(
+ 'Conflicting options detected: libcxx_library and
use_system_lib')
+ libcxx_library = None
----------------
danalbert wrote:
> Should say which one is being used if we aren't making this an error
I'll make it an error.
================
Comment at: test/lit.cfg:501
@@ +500,3 @@
+ if libcxx_library:
+ self.link_flags += [libcxx_library, '-Wl,-rpath',
+ '-Wl,' + os.path.dirname(libcxx_library)]
----------------
danalbert wrote:
> Doesn't rpath need an argument?
It gets its argument from the second `-Wl` flag but I'll join them for clarity.
================
Comment at: test/lit.cfg:597
@@ +596,3 @@
+ self.use_scan_build = self.get_lit_conf('use_scan_build')
+ if self.use_scan_build is None:
+ return
----------------
danalbert wrote:
> I don't quite understand why '' means yes, but 'true' means no. Shouldn't
> this just be
>
> if not self.use_scan_build:
> ...
> else:
> ...
>
> ? It changes the meaning slightly, but I think this behavior makes more
> sense. Am I missing something?
The semantics of `use_scan_build` are intended to be:
```
--param=use_scan_build[=<path/to/scan-build>]
```
Where the argument defaults to `scan-build` if omitted.
http://reviews.llvm.org/D5877
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
