Page MenuHomePhabricator

[flang] Add clang-tidy check for braces around if
ClosedPublic

Authored by rovka on Jun 11 2021, 2:15 AM.

Details

Summary

Flang diverges from the llvm coding style in that it requires braces
around the bodies of if/while/etc statements, even when the body is
a single statement.

This commit adds the readability-braces-around-statements check to
flang's clang-tidy config file. Hopefully the premerge bots will pick it
up and report violations in Phabricator.

We also explicitly disable the check in the directories corresponding to
the Lower and Optimizer libraries, which rely heavily on mlir and llvm
and therefore follow their coding style.

Diff Detail

Event Timeline

rovka created this revision.Jun 11 2021, 2:15 AM
rovka requested review of this revision.Jun 11 2021, 2:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2021, 2:15 AM

This should probably not be applied to all of f18; the lowering code, which interacts heavily with mlir and llvm, respects their styles.

This should probably not be applied to all of f18; the lowering code, which interacts heavily with mlir and llvm, respects their styles.

Right, makes sense. I'll look into that and try to update the patch. Thanks.

rovka updated this revision to Diff 351805.Jun 14 2021, 3:02 AM
rovka edited the summary of this revision. (Show Details)

I disabled the check for Optimizer and Lower. With the newest version of the patch, I'm seeing several violations in lib/Semantics, tools (f18, f18-parse-demo, tco) and 2 rogue ones in the runtime. I'm going to fix the ones from the runtime, but I'm not sure about the others. What's the preferred style for lib/Semantics and tools?

jeanPerier accepted this revision.Jun 14 2021, 7:24 AM

tco, fir-opt and bbc tools are lowering tools, so they currently use llvm style.

This revision is now accepted and ready to land.Jun 14 2021, 7:24 AM

I disabled the check for Optimizer and Lower. With the newest version of the patch, I'm seeing several violations in lib/Semantics, tools (f18, f18-parse-demo, tco) and 2 rogue ones in the runtime. I'm going to fix the ones from the runtime, but I'm not sure about the others. What's the preferred style for lib/Semantics and tools?

I wouldn't worry about the tools; few if any of those will survive in the llvm tree. The semantics code uses the stricter style.

This should probably not be applied to all of f18; the lowering code, which interacts heavily with mlir and llvm, respects their styles.

I thought not having readability-braces-around-statements was the llvm/mlir default?

This should probably not be applied to all of f18; the lowering code, which interacts heavily with mlir and llvm, respects their styles.

I thought not having readability-braces-around-statements was the llvm/mlir default?

Ah I misunderstood what the current patch is doing...

schweitz added inline comments.
flang/include/flang/Lower/.clang-tidy
1

Are this explicit removals really required? clang-tidy is supposed to use the .clang-tidy from the nearest parent and InheritParentConfig is not set.

rovka added a comment.Jun 15 2021, 1:30 AM

I disabled the check for Optimizer and Lower. With the newest version of the patch, I'm seeing several violations in lib/Semantics, tools (f18, f18-parse-demo, tco) and 2 rogue ones in the runtime. I'm going to fix the ones from the runtime, but I'm not sure about the others. What's the preferred style for lib/Semantics and tools?

I wouldn't worry about the tools; few if any of those will survive in the llvm tree. The semantics code uses the stricter style.

Just to clarify, by stricter style you mean *with* braces?

In any case, I'm uploading a new version with all the changes required to make flang clean with respect to this check - no braces in Lower, Optimizer and tools, braces everywhere else.

flang/include/flang/Lower/.clang-tidy
1

Actually InheritParentConfig is set to true in all of these, so we do need to explicitly remove them.

rovka updated this revision to Diff 352062.Jun 15 2021, 1:32 AM

Could you also update https://github.com/llvm/llvm-project/blob/main/flang/docs/C%2B%2Bstyle.md#layout to reflect this change? Otherwise our documentation and clang-tidy scripts are out-of-sync. Thank you!

Just to clarify, by stricter style you mean *with* braces?

Yes, I do.

klausler accepted this revision.Jun 15 2021, 9:19 AM
rovka added a comment.Jun 16 2021, 2:00 AM

Could you also update https://github.com/llvm/llvm-project/blob/main/flang/docs/C%2B%2Bstyle.md#layout to reflect this change? Otherwise our documentation and clang-tidy scripts are out-of-sync. Thank you!

Right, thanks for pointing that out! I added something, feel free to rephrase it :)

This revision was automatically updated to reflect the committed changes.