User Details
- User Since
- Nov 13 2015, 7:20 AM (341 w, 1 d)
Feb 3 2022
Overall looks good to me but please re-upload patch on top of some stable revision where failed test passes (it seems unrelated to your changes).
Jul 30 2021
Jun 30 2021
Jun 29 2021
Fix clang-tidy style warning
Replace tab with spaces
Jun 11 2021
Jun 10 2021
LGTM
Jun 9 2021
Jun 7 2021
Fix clang-tidy warning
Jun 4 2021
Jun 3 2021
@bruno and @dexonsmith thank you for the review!
Jun 2 2021
Updated HeaderMapImpl::reverseLookupFilename according to the review comment
Jun 1 2021
Rebase + try build on Windows again
Resolved comments
May 31 2021
May 27 2021
Fix forgotten comment
Split changes into NFC https://reviews.llvm.org/D103229 and the rest
May 26 2021
@alexfh, @njames93 and @thakis please take a look! I added all tests cases and put new logic behind a flag to make it as safe as possible.
Issue with diagnostics from macro expansion from third-party headers is the one of the biggest problem with deployment that we have and it cannot be properly fixed with wrappers around clang-tidy.
Added test for system like object macro
Fix linter
Fix clang-format issue
May 21 2021
Apr 19 2021
Fix wrong character
Put feature behind flag
Apr 12 2021
I think there is no sense to invent another HeaderFilterRegex like option to control which headers should be excluded. HeaderFilterRegex should be good enough because if you would like to see warning from the header, most probably you can fix code in the header or at least add a suppression.
Mar 24 2021
Mar 23 2021
Check exact list of enabled checks
Mar 15 2021
Rebase again
Rebase
Mar 9 2021
@aaron.ballman @alexfh @njames93 - friendly ping, please take a look!
Feb 27 2021
Use -DAG checks to make test stable
Feb 26 2021
@njames93 thank you for quick response and good suggestion!
Comments resolved
Feb 22 2021
@shixiao do you have plans to keep working on this diff? If not, do you have any objections if I send for review a similar diff that will eliminate the check Sources.isInMainFile and will rely on generic header filtration mechanism in clang-tidy?
Feb 10 2021
Jan 10 2021
I investigated this issue a bit under debugger. It seems that there are multiple places where resolver is used as "base" object for ifunc and resolver attributes like comdat are applied to ifunc itself. It is reasonable for aliases but not for ifunc. Unfortunately it is not something that can be changed easily. The proposal with deriving GlobalIFunc from GlobalObject makes more sense to me now.
Jan 9 2021
Oh, it seems that LTO is already relay on getBaseObject from ifunc :(
Jan 6 2021
I think it should fix immediate problem that we have and makes things more consistent in general.
Nov 18 2020
@njames93 thank you for the feedback!
- addressed comments in the diff
- stop ignoring system macro
Nov 5 2020
Fix clang-tidy warning
Nov 4 2020
Sorry, I don't think it's worth reverting and resubmitting the same changes in such a case. Commit attribution is correct and there is link to this code review.
After all, most clang-tidy users will never read commit messages and will only use --help for discovering this feature.
Nov 3 2020
Hello @@DmitryPolukhin ,
When I submitted latest via 'arc diff' my commit-message was...
Can you please help to have following commit message? Below commit message is more clear and helpful.
Sorry for inconvenience caused!
Oct 30 2020
Looks good to me.
Oct 29 2020
Oct 28 2020
I think this diff looks very close to what we need. I hope it will be the last iteration.
Oct 27 2020
Yes, it is what I meant as a simpler solution. But please add Lit test even for such trivial option.
@Hiralo, it looks like you uploaded wrong diff because your previous revision is shown as "base" version. Base revision should be clang-tidy sources without your changes.
Oct 26 2020
I'm not sure that we need additional option to read configuration from file but, if we do need, I think this diff needs some improvements + test for new option.
Oct 23 2020
Oct 22 2020
Added all module map file names
Oct 21 2020
And one more time linting
Linting diff
Jul 29 2020
Thank you for identifying and fixing!
Jul 9 2020
@aaron.ballman - thank you for the review!
Jul 8 2020
I don't understand what do you mean by "not idempotent" behavior in this case. As far as I can see GlobalIFunc doesn't implement own getBaseObject (and it is not virtual) so calling getBaseObject on the IFunc should return null same as calling it on Alias-to-IFunc. Calling getbaseObject on Alias-to-IFunc will recursively call it on IFunc that will return null that will be propagated, isn't it? So in my opinion computeAliasSummary should handle null without crash because other places have checks for null returned from getBaseObject.
Jul 7 2020
I did a bit of archeology and it turns out that getBaseObejct was part of moved from GlobalAlias to GlobalIndirectSymbol in https://github.com/llvm/llvm-project/commit/95549497ec8b5269f0439f12859537b7371b7c90
It looks like the simplest solution is to handle nullptr from getBaseObejct in computeAliasSummary...
getBaseObject is only small part of code sharing, the majority of the code is outside the class in common code that handles both GlobalIFunc and GlobalAliases in the same way. Here they should be treated differently so, IMHO, there is no need in changing the inheritance but GlobalIFunc should be handles as a special case here.
Jul 6 2020
@njames93 and @aaron.ballman - please take a look to this diff. Multiline replacements in YAML are broken and cannot be applied correctly.
Jun 24 2020
Fix test