This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Fix c_str() removal and cast addition when re-ordering arguments
ClosedPublic

Authored by mikecrowe on Jul 30 2023, 7:13 AM.

Details

Summary

The modernize-use-std-print check would get confused if it had to
re-order field-width and precision arguments at the same time as adding
casts or removing calls to c_str().

Fix this by tracking the argument indices and combining c_str() removal
with argument re-ordering. Add missing test cases to lit check.

Fixes https://github.com/llvm/llvm-project/issues/64033

Diff Detail

Event Timeline

mikecrowe created this revision.Jul 30 2023, 7:13 AM
Herald added a project: Restricted Project. · View Herald Transcript
mikecrowe requested review of this revision.Jul 30 2023, 7:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2023, 7:13 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Fixing this was rather messier than I expected, but this solution does seem to work for the test cases. Here's my original commit message before I trimmed it, in case it provides any more insight.

[clang-tidy] Fix c_str() removal and cast addition when re-ordering arguments

The modernize-use-std-print check would get confused if it had to
re-order field-width and precision arguments at the same time as adding
casts or removing calls to c_str(). For example applying the check with
StrictMode enabled to:

 printf("%*s=%*d\n", 4, s.c_str(), 3, ui);

yields:

 std::println("{:>{}}={:{}}", s.c_str(), s4, ui, static_cast<int>(3));

rather than the expected:

 std::println("{:>{}}={:{}}", s, 4, static_cast<int>(ui), 3);

Fix this by:

- storing the ArgIndex rather than the Expr pointer for any arguments
  that need casts to be added in ArgFixes so that the index can be
  modified when the arguments are re-ordered. Use a struct rather than a
  tuple for clarity.

- Making applyFixes do argument re-ordering first, but also taking care
  of c_str() removal at the same time if necessary. Update the argument
  index of any ArgFixes too.

- Apply the ArgFixes afterwards.

- Finally apply and c_str() removals that remain.

- Add lit check test cases for the combinations of argument reordering,
  casts and c_str() removal. This required moving the definition of
  struct iterator earlier in use-std-print.cpp.
mikecrowe updated this revision to Diff 545446.Jul 30 2023, 8:16 AM

Rebase on top of D156616 and add more tests

mikecrowe updated this revision to Diff 546414.Aug 2 2023, 5:12 AM

Reinstate version I incorrectly replaced with D154287

@PiotrZSL, I think that this is quite an important fix since without it the check completely mangles the code. Should it be put in the 17.x release branch too?

PiotrZSL accepted this revision.Aug 28 2023, 10:32 AM

@PiotrZSL, I think that this is quite an important fix since without it the check completely mangles the code. Should it be put in the 17.x release branch too?

If issue happens only when StrictMode is set to True, then I wouldn't worry too much, and then this could wait for Clang 18 that is just half year away.

This revision is now accepted and ready to land.Aug 28 2023, 10:32 AM

@PiotrZSL, I think that this is quite an important fix since without it the check completely mangles the code. Should it be put in the 17.x release branch too?

If issue happens only when StrictMode is set to True, then I wouldn't worry too much, and then this could wait for Clang 18 that is just half year away.

The removal of c_str() happens even without StrictMode set to True.

The removal of c_str() happens even without StrictMode set to True.

I still think we will be fine. There are not many situations when arguments need to be reorder. And most probably not many users will change default configuration.
LLVM 17 is already past RC3, this issue is not critical, nor a regression, user can always disable check, or just manually fix few problematic cases in code.
Additionally user could always take "main" version, and use it just to apply fixes from this check.

Additionally user could always take "main" version, and use it just to apply fixes from this check.

OK. Can this land on main then?

Thanks.

Thanks for the review and landing this.

Mike.