This is an archive of the discontinued LLVM Phabricator instance.

[Utils] Reuse argument variable names in the body
ClosedPublic

Authored by jdoerfert on Nov 1 2019, 10:54 AM.

Details

Summary

If we have int foo(int a) { return a; } and we run with --function-signature
enabled, we want a single variable declaration for a which is reused
later.

Diff Detail

Event Timeline

jdoerfert created this revision.Nov 1 2019, 10:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2019, 10:54 AM
Herald added a subscriber: bollu. · View Herald Transcript

Can you show the effect by regenerating some test?

Can you show the effect by regenerating some test?

Take line 8 here https://reviews.llvm.org/D68766#change-4geuqNmmUCZL

`; ARGPROMOTION-NEXT:    ret i32 [[X_VAL]]`

It uses [[X_VAL]] which is an argument of the function.
Without this check line would look like this:

`; ARGPROMOTION-NEXT:    ret i32 [[X:.%*]]`

which has no relationship to the argument.

lebedev.ri requested changes to this revision.Dec 6 2019, 6:05 AM

Now that i believe there's some testing infra for these utils, add/update tests for this?

This revision now requires changes to proceed.Dec 6 2019, 6:05 AM
jdoerfert updated this revision to Diff 235674.Dec 30 2019, 8:59 PM

Add and adjust regression tests

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

lebedev.ri accepted this revision.Dec 30 2019, 11:32 PM
lebedev.ri added subscribers: spatel, RKSimon.

This is not opt-in and will cause a new wave of test regeneration,
BUT @signature suggests to me that this could be viewed as a fix and not a new feature.

Any other opinions, @spatel @RKSimon ?
(please don't commit until more generally-favorable review outcome)

This revision is now accepted and ready to land.Dec 30 2019, 11:32 PM

Add an explicit test without --function-signature

This is not opt-in and will cause a new wave of test regeneration,

This is "opt-in" with the --function-signature option. I added an explicit test and the basic test should cover that as well, e.g., only the .funcsig. version is affected.

lebedev.ri accepted this revision.Dec 30 2019, 11:54 PM

Right, we couldn't have already matched function arg names because we don't normally do that.

Unit tests: fail. 61155 tests passed, 1 failed and 728 were skipped.

failed: LLVM.tools/UpdateTestChecks/update_test_checks/argument_name_reuse.test

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

This revision was automatically updated to reflect the committed changes.