Page MenuHomePhabricator

[DebugInfo] run clang-format on some unformatted files
ClosedPublic

Authored by ljmf00 on Nov 10 2021, 8:25 AM.

Details

Summary

This trivial patch runs clang-format on some unformatted files before doing logic changes and prevent hard to review diffs.

Diff Detail

Event Timeline

ljmf00 created this revision.Nov 10 2021, 8:25 AM
ljmf00 requested review of this revision.Nov 10 2021, 8:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2021, 8:25 AM
probinson accepted this revision.Nov 10 2021, 10:39 AM
probinson added a subscriber: probinson.

In general doing a clang-format before extensive work on some files is an obvious NFC, but I can see why you might have wanted some review on this.

LGTM, although the one thing I'd really like you to try is reflowing that one awkward function description. All the other trivial spacing stuff is purely optional.

llvm/include/llvm/IR/DebugInfoMetadata.h
610

It looks like clang-format preserved the spaces around the first asterisk (MDString * Filename) but handled the next case correctly (MDString *Directory). Can you manually remove the space before Filename and see if clang-format leaves it that way?

1780

This reflowing is awkward. Try moving each \p to a new line, and maybe indent a couple of spaces, see if that causes clang-format to behave better. You might need an empty /// line before the \p lines.

2145

Try removing the space before Scope and see if clang-format will preserve that.

2149

Try removing the space before Scope and see whether clang-format will preserve that.

2196

And another Scope with an incorrect space.

2311

Man, these are everywhere. I'm going to stop pointing out each one.

llvm/lib/Bitcode/Reader/MetadataLoader.cpp
1677

I think putting an = at the end of DIFlagMainSubprogram will avoid that space.

llvm/lib/IR/DIBuilder.cpp
786

/*AlignInBits=*/

This revision is now accepted and ready to land.Nov 10 2021, 10:39 AM

In general doing a clang-format before extensive work on some files is an obvious NFC, but I can see why you might have wanted some review on this.

LGTM, although the one thing I'd really like you to try is reflowing that one awkward function description. All the other trivial spacing stuff is purely optional.

Thanks for the review! I'm aware that such code style patches don't need approval, according to the LLVM developers policy, but since I'm currently an outside contributor without commit access, I'm obligated to do it. I think these are also good for me to get used to this nit things clang-format is not able to fix, although I don't expect any extensive review.

ljmf00 updated this revision to Diff 386298.Nov 10 2021, 1:15 PM

@probinson Can you check now? If it looks good, can you land it, please?

llvm/include/llvm/IR/DebugInfoMetadata.h
1780

I added a blank /// similar to the above documentation.

2145

clang-format insists on this, because of the macro.

2196

It is very weird because the second parameter doesn't get indented.

@probinson Can you check now? If it looks good, can you land it, please?

Sure. It looks like your email address is <contact@lsferreira.net> is that correct? We do want to be careful about attributions.

@probinson Can you check now? If it looks good, can you land it, please?

Sure. It looks like your email address is <contact@lsferreira.net> is that correct? We do want to be careful about attributions.

Yes, it is.

This revision was automatically updated to reflect the committed changes.