[analyzer] Relative include paths to --sysroot #3259
Conversation
bruntib
added
CLI 💻
| @@ -722,48 +722,48 @@ def __collect_transform_include_opts(flag_iterator, details): | |||
|
|
|||
| m = COMPILE_OPTIONS_MERGED.match(flag_iterator.item) | |||
There was a problem hiding this comment.
"matches" or any non one-character variable name please.
There was a problem hiding this comment.
It is a commonly used variable name (see the official documentation: https://docs.python.org/3/library/re.html) like the name f for file objects. So I think it is better to leave it as it but it's up to @bruntib.
| if together: | ||
| details['analyzer_options'].append(flag + param) | ||
| else: | ||
| details['analyzer_options'].extend([flag, param]) |
There was a problem hiding this comment.
What do you think to shorten the source code like this:
| if together: | |
| details['analyzer_options'].append(flag + param) | |
| else: | |
| details['analyzer_options'].extend([flag, param]) | |
| details['analyzer_options'].extend( | |
| [f"{flag}{param}"] if together else [flag, param]) |
| @@ -722,48 +722,48 @@ def __collect_transform_include_opts(flag_iterator, details): | |||
|
|
|||
| m = COMPILE_OPTIONS_MERGED.match(flag_iterator.item) | |||
There was a problem hiding this comment.
It is a commonly used variable name (see the official documentation: https://docs.python.org/3/library/re.html) like the name f for file objects. So I think it is better to leave it as it but it's up to @bruntib.
| if m: | ||
| flag = m.group(0) | ||
| together = len(flag) != len(flag_iterator.item) | ||
| if not m: |
There was a problem hiding this comment.
I like this change 😊
| # --sysroot=/path/to/include as | ||
| # --sysroot /path/to/include | ||
| # which is a valid format too. | ||
| if param.startswith("="): |
There was a problem hiding this comment.
Above in the if branch we check this on this way: param[0] != '='. Here we are using the startswith function. Can we use a common way to check this?
| '-iquote', '-isysroot', '-isystem', | ||
| '-iwithprefix', '-iwithprefixbefore', '-sysroot', | ||
| '--sysroot'] | ||
| if flag in flags_with_path and ('sysroot' in flag or param[0] != '='): |
There was a problem hiding this comment.
The documentation mention the = only for the following options:
-I dir
-iquote dir
-isystem dir
-idirafter dir
See: https://gcc.gnu.org/onlinedocs/gcc/Directory-Options.html.
My question is that here we handle this not only for these options but for others as well like for iwithprefix or imultilib. What do you think, is it a problem or not?
There was a problem hiding this comment.
No, it is not a problem, because the path can't start with = at these flags, because it wouldn't compile. However the paths at these flags shouldn't be converted to absolute, so I removed them from the list. Good point, thank you.
If a -I flag is followed by a path starting with "=" sign, it means that it's relative to --sysroot. In CodeChecker we tend to convert all paths to absolute, however, these should be skipped. For example: -I =/usr/include/my_lib
If a -I flag is followed by a path starting with "=" sign, it means that
it's relative to --sysroot. In CodeChecker we tend to convert all paths
to absolute, however, these should be skipped. For example:
-I =/usr/include/my_lib