Allow --file and skipfile option to be given together and analyze header file #3616
Conversation
csordasmarton
commented
Mar 2, 2022
csordasmarton
commented
csordasmarton
added
enhancement 🌟
test ☑️
| @@ -107,3 +107,18 @@ def __call__(self, source_file_path: str) -> bool: | |||
| Check if the given source should be skipped. | |||
| """ | |||
| return self.should_skip(source_file_path) | |||
|
|
|||
|
|
|||
| class SkipListHandlers(list): | |||
There was a problem hiding this comment.
This class has two methods doing the same thing. Couldn't we eliminate one of them?
There was a problem hiding this comment.
The above class also implemented these methods. For now I added a FIXME to remove one of the method from both classes.
| header_file_extensions=( | ||
| '.h', '.hh', '.H', '.hp', '.hxx', '.hpp', '.HPP', '.h++', '.tcc') |
There was a problem hiding this comment.
This shouldn't be a parameter of this function.
There was a problem hiding this comment.
Okay, I moved it to a separate variable outside of this function.
| skip_file_content = "" | ||
| file_paths = [] | ||
| for file_filter in file_filters: | ||
| file_paths.append(file_filter) |
There was a problem hiding this comment.
The goal of the function would be clearer if its result wouldn't include these header files. The function's name is get_source_file_paths() where "source" indicates that non-headers are returned. At least we are using "source file" for non-headers conventionally throughout in our naming conventions. The list of headers could be concatenated to the source files at the caller.
I'm not sure, but I'm thinking if this function is necessary at all. Couldn't tu_collector.get_dependent_sources() be used directly? As far as I can see, the added behavior of this function is to check only existing headers in the given list. What is the reason of this restriction? The result of this function goes to a skipfile where it is not a big problem if the file doesn't exist.
There was a problem hiding this comment.
I renamed this function to get_affected_file_paths.
| """ | ||
| skip_file_content = "" | ||
| file_paths = [] |
There was a problem hiding this comment.
| file_paths = [] | |
| file_paths = set() |
There was a problem hiding this comment.
I used list to keep the order of the items. I added a comment to highlight it.
| LOG.debug("Dependent source files: %s", | ||
| ', '.join(dependent_sources)) | ||
|
|
||
| file_paths.extend(dependent_sources) |
There was a problem hiding this comment.
| file_paths.extend(dependent_sources) | |
| file_paths.update(dependent_sources) |
| Initialize and return a list of skiplist handlers if | ||
| there is a skip list file in the arguments or files options is provided. | ||
| """ | ||
| skip_handlers = SkipListHandlers() |
There was a problem hiding this comment.
Wouldn't concatenation of two skipfile contents be equivalent with the usage of SkipListHandlers()? Because I think it wouldn't be necessary introducing SkipListHandlers() if they are.
There was a problem hiding this comment.
I described the problem why we can't use a single skip file in the following issue: #3595 (comment)
The CodeChecker VSCodePlugin uses the `--file` parameter to analyze single files. Large projects load in their configuration using the `--config` parameter and if there is a `-i skipfile` given in the config, `CodeChecker analyze` call drops an error. This patch will allow `-i skipfile` and `--file` to be given together.
csordasmarton
force-pushed
the
file_opt_with_skipfile
branch
2 times, most recently
from
f832a75 to
ec491fb
Compare
csordasmarton
force-pushed
the
file_opt_with_skipfile
branch
from
ec491fb to
9d6988c
Compare
