This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Improve

Authored by ccotter on Jan 10 2023, 11:49 PM.


Summary now find and renames the test file.
also will now use 'git mv', so the developer no longer has to manually
add the file after running the script.

Diff Detail

Event Timeline

ccotter created this revision.Jan 10 2023, 11:49 PM
Herald added a project: Restricted Project. · View Herald Transcript
ccotter requested review of this revision.Jan 10 2023, 11:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2023, 11:49 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I tested with where I renamed my new check, but noticed did not rename or update the check name in the test file. The result of running the updated script against my tool is on github:

Thanks for the fix! Some minor comments/questions.


I'm not sure it's the responsibility of this check to do git stuff, there may be use cases where this is not wanted / users might not expect this behavior. Introducing a dependency to git might also make this script harder to unit test in the future.

Thus I think it'd be better to keep the original behavior so the script has one single responsibility. What do you think @njames93 ?


Why is this change needed? I'd expect only line 99 to be needed to fix moving the test.

ccotter added inline comments.Jan 11 2023, 12:16 AM

Ah, it's not needed, but I thought it was a bit cleaner to have both the globs take the same approach and be recursive, avoiding the for-loop over the subdirectories.

carlosgalvezp added inline comments.Jan 11 2023, 12:22 AM

Ah yes I see it now, yes it's good to keep the consistency, thanks!

ccotter updated this revision to Diff 488169.Jan 11 2023, 5:34 AM
  • revert git changes
ccotter marked an inline comment as done.Jan 11 2023, 5:35 AM
ccotter added inline comments.

I wasn't sure about this - reverted this. Single responsibility is better.

carlosgalvezp accepted this revision.Jan 11 2023, 5:50 AM

LGTM, thanks for the fix! Let's give a few days for other reviewers to get a look.

This revision is now accepted and ready to land.Jan 11 2023, 5:50 AM
njames93 added inline comments.Jan 12 2023, 12:35 PM

Yeah we shouldn't be messing with git indexes.

This revision was automatically updated to reflect the committed changes.
ccotter marked an inline comment as done.Jan 23 2023, 1:44 PM

Thanks for reviewing and merging my recent changes!