Thanks @MaskRay.

Thanks @MaskRay. The patch is updated.

Thanks @MaskRay, I have updated the patch following your suggestions.

Thanks @simon_tatham and @MaskRay for the quick code review.

The changes include:

Thanks @MaskRay, the fix is fine for me.

Gentle ping ...

Analysis the issue using the above simple example:

A simple test to reproduce the issue:

% cat main.cpp
#include "1.h"
#include "2.h"
int foo();

Thanks Zequan. I have added two tests following your suggestion.

Gentle ping ...

@MaskRay, thanks again. I have marked comments as done.

Removed '2>&1' from 'RUN: %python'.

@MaskRay, please let me know if there are any further changes required. Thanks.

@tejohnson and @MaskRay, thanks for reviewing the patch.

Thanks for your comments.

Update the patch following the reviewers' comments, which include:

I think I might be able to incorporate checkCacheCapacity into pruneCache. I am working on this.

Gentle ping ..

The changes include:

Updating the correct patch.

Thanks Teresa and Fangrui for the comments. Sorry for the delay in updating the patch. I have been trying to find a way to get the warning invoked by all cache users.

Update the patch following the reviewers comments, which include:

Thanks Paul.

Use Args.addOptInFlag instead of Args.hasFlag, thanks.

Thanks David, I have modified the test following your advice.

Hi Jonas and Nico,

Hi Jonas,

I've reverted for now - I took a quick look at the sanitizer test and couldn't grok whats going wrong, will try again when I'm awake :)

It seems that this commit has caused several test failures.

Hi Petr Hosek,

The previous patch doesn’t support the sequential commands, for example “mkdir existing_folder || true”. This caused sanitizer-windows test failed. The patch has been updated to support the sequential commands.

Reverted rL318911 since it broke the sanitizer-windows.

The test of MC/AsmParser/preserve-comments-crlf.s failed since it uses the unsupported diff option --strip-trailing-cr. I updated the internal diff shell command to support this option.

Optimised the patch by using python ‘getopt’ module.

Updated the builtin diff shell to support ‘-u’, ‘-b’ and ‘-w’ options. Now, all llvm, and clang tests passed with both Python 2 and Python 3.

1. I don't see any tests for the case where the commands *succeed*. Only for the case where they fail. Shouldn't we have some tests that verify the commands actually work?

The patch updated the test (valid-shell.txt) where lit internal shell commands succeeded.

Update: I actually do see where you're testing that they work, but I'm not convinced that the tests are sufficient. For several reasons.

1. it doesn't test that the builtin command is actually run versus the OS command. It only tests that whatever command is run doesn't fail.

2. It doesn't test that the result of the command is as expected, it only tests that it doesn't return an error. For a somewhat unrealistic illustration of this, imagine that all the commands just returned success without actually doing anything. The test as it is written now would pass.

2. Now that we have upwards of 6 builtin commands, I'm starting to think they should be separated into their own file. For example, lit/test/ShellBuiltins.py We could test them via unittests (e.g. see how BooleanExpression.py implements its unit tests).

What do you think?

In D25086#572183, @vsk wrote:

@MaggieYi ping, do you have the bandwidth to update this patch to use the new CoverageMapping::load API? If not, I can take a shot at it.

In D24687#546833, @vsk wrote:

I'd prefer to just tackle this issue in this patch. IMO it would be better to drop (or at least defer) the other changes.

Ok, I have updated the patch following your advice.

I agree with you and abandon this patch.

In D24367#539047, @vsk wrote:

On a low level, I'm concerned about the unnecessary quadratic behavior in this patch. If "N = |SourceFiles|", then renderPageNavigation() does O(N) work and it's called O(N) times. Given that llvm+clang has over 2,400 source files, this could actually be noticeable. We already get user reports about llvm-cov being slow...

Thanks for your pseudocode . Two issues have been fixed.

Thanks for your explanation. I have updated the patch following your advice.

In D23277#526594, @vsk wrote:

Thanks for splitting things up :).

I'm not sure that the javascript bit is needed. It looks like a way to remove/disable a link, but I don't see any issue with leaving the link active.

Reused the existing native_separator.c test to check fully covered source file case.

Thanks, the test has been added.

Thanks Vedant, the patch has been updated following your comments.

Updating the patch to support the "Goto first zero count" feature.