Merge CodeChecker default and user defined arguments #88
Conversation
| def _merge_options(default, custom): | ||
| """ | ||
| Merge command line arguments so that default options can be overridden | ||
| """ | ||
| final = [] | ||
| args_set = [] | ||
| for item in custom: | ||
| args_set.append(item.split(" ")[0].split("=")[0]) | ||
| final.append(item) | ||
| for option in default: | ||
| if option.split(" ")[0].split("=")[0] not in args_set: | ||
| final.append(option) | ||
| return final |
There was a problem hiding this comment.
Can't we do something more elegant? Are depsets appropriate here, which can leverage uniquing?
There was a problem hiding this comment.
Depsets are not appropriate here, since what I really want to solve is "--analyzers" showing up twice if the user defines it too.
The reason to do this at all is to be able to know which analyzers will run, and thus what output plist files I need to declare.
Since the whole argument is as: "--analyzers clangsa clang-tidy" and a user-defined could be "--analyzers cppcheck", these two strings are different, so they would both go into the depset. We could add only the first part "--analyzers" to the depset; this way, it would clear the duplicate, but how do we know what the list of activated analyzers is?
We could restrict the merge to only consider "--analyzers" arguments, if that would be more elegant, and leave other parameters alone.
There was a problem hiding this comment.
I just noticed that a user defining an argument multiple times could cause problems.
There was a problem hiding this comment.
Meaning enabling multiple checkers like -e cplusplus -e core etc?
There was a problem hiding this comment.
That would actually be fine. I mean my goal with this was to have at most a single --analyzer argument, so I can use its content easily to determine how many output files I should declare in Bazel. But if the user is able to declare multiple --analyzer arguments, I'm still in the dark.
There was a problem hiding this comment.
In that case, the last --analyzers argument will be used (which is a common technique to make scripting, and appending easier, just need to append some new flags at the end).
There was a problem hiding this comment.
Also, the previous loop preserves all user declared options, right? Maybe its not a good omen to mess with that, even if it contains multiple flags of which only the last will be used. Is this a big issue for you?
There was a problem hiding this comment.
It's not a huge issue, just makes this patch basically pointless.
furtib
force-pushed
the
merge-codechecker-arguments
branch
from
79bce67 to
fb00188
Compare
Szelethus
left a comment
Szelethus
left a comment
There was a problem hiding this comment.
I actually quite like the patch as it is.
src/per_file.bzl
Outdated
| final = [] | ||
| args_set = [] | ||
| for item in custom: | ||
| args_set.append(item.split(" ")[0].split("=")[0]) |
There was a problem hiding this comment.
How about we create a getter function to make this more talkative:
def get_option_name(option: str):
return option.split(" ")[0].split("=")[0]| def _merge_options(default, custom): | ||
| """ | ||
| Merge command line arguments so that default options can be overridden | ||
| """ | ||
| final = [] | ||
| args_set = [] | ||
| for item in custom: | ||
| args_set.append(item.split(" ")[0].split("=")[0]) | ||
| final.append(item) | ||
| for option in default: | ||
| if option.split(" ")[0].split("=")[0] not in args_set: | ||
| final.append(option) | ||
| return final |
There was a problem hiding this comment.
Also, the previous loop preserves all user declared options, right? Maybe its not a good omen to mess with that, even if it contains multiple flags of which only the last will be used. Is this a big issue for you?
|
You mention this "previous loop" frequently. I'm not sure what you mean. Previously, we concatenated the arguments. Also, since we don't want to mess with the user-provided arguments, this PR seems to be completely pointless. We should (if possible) unmerge #87. |
|
I actually disagree. Having the user given arguments least temprered me is beneficial, which this PR actually achieves,, but I agree that it has fallen off of importance. |
furtib
force-pushed
the
merge-codechecker-arguments
branch
from
fb00188 to
654ffc5
Compare
furtib
force-pushed
the
merge-codechecker-arguments
branch
from
6851ecf to
750fee7
Compare
2 participants
Why:
Currently, the default and user-defined arguments get mushed together.
What:
Added a function merging the default and custom rules based on the first word of the argument. Like in the case of
--analyzers clang-tidy, I would compare whether user-defined arguments contain a "--analyzers" flag.Addresses:
Fixes: #86
Depends on:
#87