Dan Liew via llvm-dev
2016-Jun-02 05:51 UTC
[llvm-dev] -Wmisleading-indentation violations
Hi,
I was building LLVM with gcc 6.1.1 recently and it was spitting out
some warnings relating to misleading indention that caught my eye.
This wasn't a fresh build so I may have missed some. I've CC'ed the
authors of the potentially misleading lines so they can decide what do
about the warnings (if anything).
I'm wondering if clang-format is making some inappropriate choices
here or these are just genuine mistakes.
Anyway here are the ones I saw in passing.
```
/home/dsl11/dev/llvm-upstream/src/lib/MC/MCParser/DarwinAsmParser.cpp:
In member function ‘bool
{anonymous}::DarwinAsmParser::parseVersionMin(llvm::StringRef,
llvm::SMLoc)’:
/home/dsl11/dev/llvm-upstream/src/lib/MC/MCParser/DarwinAsmParser.cpp:962:3:
warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
if (Update > 255 || Update < 0)
^~
/home/dsl11/dev/llvm-upstream/src/lib/MC/MCParser/DarwinAsmParser.cpp:964:5:
note: ...this statement, but the latter is misleadingly indented
as if it is guarded by the ‘if’
Lex();
^~~
```
```
/home/dsl11/dev/llvm-upstream/src/lib/Transforms/Scalar/LoopStrengthReduce.cpp:
In member function ‘void {anonymous}::Cost::RateRegister(const
llvm::SCEV*, llvm::SmallPtrSetImpl<const llvm::SCEV*>&, const
llvm::Loop*, llvm::ScalarEvolution&, llvm::DominatorTree&)’:
/home/dsl11/dev/llvm-upstream/src/lib/Transforms/Scalar/LoopStrengthReduce.cpp:943:3:
warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
if (!isa<SCEVUnknown>(Reg) &&
^~
/home/dsl11/dev/llvm-upstream/src/lib/Transforms/Scalar/LoopStrengthReduce.cpp:950:5:
note: ...this statement, but the latter is misleadingly
indented as if it is guarded by the ‘if’
NumIVMuls += isa<SCEVMulExpr>(Reg) &&
^~~~~~~~~
```
and
```
/home/dsl11/dev/llvm-upstream/src/lib/Target/AMDGPU/R600MachineScheduler.cpp:
In member function ‘llvm::R600SchedStrategy::AluKind llvm::R600S
chedStrategy::getAluKind(llvm::SUnit*) const’:
/home/dsl11/dev/llvm-upstream/src/lib/Target/AMDGPU/R600MachineScheduler.cpp:225:3:
warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
if (TII->isTransOnly(MI))
^~
/home/dsl11/dev/llvm-upstream/src/lib/Target/AMDGPU/R600MachineScheduler.cpp:228:5:
note: ...this statement, but the latter is misleadingly in
dented as if it is guarded by the ‘if’
switch (MI->getOpcode()) {
^~~~~~
```
and
```
/home/dsl11/dev/llvm-upstream/src/lib/AsmParser/LLParser.cpp: In
member function ‘bool llvm::LLParser::ParseTopLevelEntities()’:
/home/dsl11/dev/llvm-upstream/src/lib/AsmParser/LLParser.cpp:260:34:
warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
if (ParseUseListOrderBB()) return true; break;
^~
/home/dsl11/dev/llvm-upstream/src/lib/AsmParser/LLParser.cpp:260:74:
note: ...this statement, but the latter is misleadingly indented as if
it
is guarded by the ‘if’
if (ParseUseListOrderBB()) return true; break;
^~~~~
```
and
```
/home/dsl11/dev/llvm-upstream/src/tools/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp:
In member function ‘void {anonymous}::ObjCDeal
locChecker::diagnoseMissingReleases(clang::ento::CheckerContext&) const’:
/home/dsl11/dev/llvm-upstream/src/tools/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp:532:5:
warning: this ‘if’ clause does not guard
... [-Wmisleading-indentation]
if (SelfRegion != IvarRegion->getSuperRegion())
^~
/home/dsl11/dev/llvm-upstream/src/tools/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp:535:7:
note: ...this statement, but the latter
is misleadingly indented as if it is guarded by the ‘if’
const ObjCIvarDecl *IvarDecl = IvarRegion->getDecl();
```
and
```
/home/dsl11/dev/llvm-upstream/src/tools/clang/lib/Basic/Targets.cpp:
In member function ‘virtual void
{anonymous}::AMDGPUTargetInfo::setSupportedOpenCLOpts()’:
/home/dsl11/dev/llvm-upstream/src/tools/clang/lib/Basic/Targets.cpp:2129:6:
warning: this ‘if’ clause does not guard... [-Wmisleading-indentat
ion]
if (GPU >= GK_SOUTHERN_ISLANDS)
^~
/home/dsl11/dev/llvm-upstream/src/tools/clang/lib/Basic/Targets.cpp:2131:8:
note: ...this statement, but the latter is misleadingly indented a
s if it is guarded by the ‘if’
Opts.cl_khr_int64_base_atomics = 1;
^~~~
```
HTH,
Dan.
Duncan P. N. Exon Smith via llvm-dev
2016-Jun-02 06:02 UTC
[llvm-dev] -Wmisleading-indentation violations
> On 2016-Jun-01, at 22:51, Dan Liew <dan at su-root.co.uk> wrote: > > ``` > /home/dsl11/dev/llvm-upstream/src/lib/AsmParser/LLParser.cpp: In > member function ‘bool llvm::LLParser::ParseTopLevelEntities()’: > > /home/dsl11/dev/llvm-upstream/src/lib/AsmParser/LLParser.cpp:260:34: > warning: this ‘if’ clause does not guard... [-Wmisleading-indentation] > > if (ParseUseListOrderBB()) return true; break; > > ^~ > > /home/dsl11/dev/llvm-upstream/src/lib/AsmParser/LLParser.cpp:260:74: > note: ...this statement, but the latter is misleadingly indented as if > it > > is guarded by the ‘if’ > > if (ParseUseListOrderBB()) return true; break; > > ^~~~~ > ```LLParser::ParseTopLevelEntities() hasn't been clang-formatted. The style is one case per line (and every case uses the exact same pattern: `if`, `return true`, `break`). I think the density this provides makes the code easier to read than clang-format's preference, but maybe a local macro would be better? -- #define DISPATCH_TOP_LEVEL_ENTITY(token, parser) \ case lltok::token: \ if (parser) \ return true; \ break; DISPATCH_TOP_LEVEL_ENTITY(kw_declare, ParseDeclare()) DISPATCH_TOP_LEVEL_ENTITY(kw_define, ParseDefine()) ... #undef DISPATCH_TOP_LEVEL_ENTITY --
Liu, Yaxun (Sam) via llvm-dev
2016-Jun-02 14:37 UTC
[llvm-dev] -Wmisleading-indentation violations
/home/dsl11/dev/llvm-upstream/src/tools/clang/lib/Basic/Targets.cpp:
In member function ‘virtual void
{anonymous}::AMDGPUTargetInfo::setSupportedOpenCLOpts()’:
/home/dsl11/dev/llvm-upstream/src/tools/clang/lib/Basic/Targets.cpp:2129:6:
warning: this ‘if’ clause does not guard... [-Wmisleading-indentat
ion]
if (GPU >= GK_SOUTHERN_ISLANDS)
^~
/home/dsl11/dev/llvm-upstream/src/tools/clang/lib/Basic/Targets.cpp:2131:8:
note: ...this statement, but the latter is misleadingly indented a
s if it is guarded by the ‘if’
Opts.cl_khr_int64_base_atomics = 1;
^~~~
```
This issue has been taken care of by http://reviews.llvm.org/D20388 . Thanks.
Sam
Dan Liew via llvm-dev
2016-Jun-02 17:42 UTC
[llvm-dev] -Wmisleading-indentation violations
On 1 June 2016 at 23:02, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:> >> On 2016-Jun-01, at 22:51, Dan Liew <dan at su-root.co.uk> wrote: >> >> ``` >> /home/dsl11/dev/llvm-upstream/src/lib/AsmParser/LLParser.cpp: In >> member function ‘bool llvm::LLParser::ParseTopLevelEntities()’: >> >> /home/dsl11/dev/llvm-upstream/src/lib/AsmParser/LLParser.cpp:260:34: >> warning: this ‘if’ clause does not guard... [-Wmisleading-indentation] >> >> if (ParseUseListOrderBB()) return true; break; >> >> ^~ >> >> /home/dsl11/dev/llvm-upstream/src/lib/AsmParser/LLParser.cpp:260:74: >> note: ...this statement, but the latter is misleadingly indented as if >> it >> >> is guarded by the ‘if’ >> >> if (ParseUseListOrderBB()) return true; break; >> >> ^~~~~ >> ``` > > LLParser::ParseTopLevelEntities() hasn't been clang-formatted. The > style is one case per line (and every case uses the exact same pattern: > `if`, `return true`, `break`).I think GCC is warning about this particular one and not the others because the ``case`` and the ``if`` are on their own lines. I.e. ``` case lltok::kw_uselistorder: if (ParseUseListOrder()) return true; break; case lltok::kw_uselistorder_bb: if (ParseUseListOrderBB()) return true; break; ``` GCC does not warn about the ``lltok::kw_uselistorder`` case (and similarly formatted cases above) but it is warning about the ``lltok::kw_uselistorder_bb`` case.> I think the density this provides makes the code easier to read than > clang-format's preference, but maybe a local macro would be better? > -- > #define DISPATCH_TOP_LEVEL_ENTITY(token, parser) \ > case lltok::token: \ > if (parser) \ > return true; \ > break; > DISPATCH_TOP_LEVEL_ENTITY(kw_declare, ParseDeclare()) > DISPATCH_TOP_LEVEL_ENTITY(kw_define, ParseDefine()) > ... > #undef DISPATCH_TOP_LEVEL_ENTITY > --Up to you. Personally I dislike using macros, especially here as the only reason for doing it would be to suppress a warning. To suppress this particular warning you could reindent or use braces for the case that doesn't fit on a single line. You could also use a ``#pragma GCC ...`` to push and pop ignoring ``-Wmisleading-indentation`` in this particular region. Whatever you find most tasteful I guess. Dan.
Dan Liew via llvm-dev
2016-Jun-02 17:52 UTC
[llvm-dev] -Wmisleading-indentation violations
Whilst I'm here I thought I would just do a clean build of LLVM and
Clang to see if any more warnings of this sort appeared. Only one
other popped up.
```
/home/dsl11/dev/llvm-upstream/src/tools/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:
In member function ‘void
{anonymous}::ArrayBoundCheckerV2::checkLocation(clang::ento::SVal,
bool, const clang::Stmt*, clang::ento::CheckerContext&) const’:
/home/dsl11/dev/llvm-upstream/src/tools/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:160:7:
warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
if (state->isTainted(rawOffset.getByteOffset()))
^~
/home/dsl11/dev/llvm-upstream/src/tools/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:162:9:
note: ...this statement, but the latter is misleadingly indented as if
it is guarded by the ‘if’
return;
^~~~~~
```
HTH,
Dan.
Dan Liew via llvm-dev
2016-Jun-02 17:58 UTC
[llvm-dev] -Wmisleading-indentation violations
Using the correct e-mail address this time.> ``` > /home/dsl11/dev/llvm-upstream/src/lib/Transforms/Scalar/LoopStrengthReduce.cpp: > In member function ‘void {anonymous}::Cost::RateRegister(const > llvm::SCEV*, llvm::SmallPtrSetImpl<const llvm::SCEV*>&, const > llvm::Loop*, llvm::ScalarEvolution&, llvm::DominatorTree&)’: > > /home/dsl11/dev/llvm-upstream/src/lib/Transforms/Scalar/LoopStrengthReduce.cpp:943:3: > warning: this ‘if’ clause does not guard... [-Wmisleading-indentation] > > if (!isa<SCEVUnknown>(Reg) && > > ^~ > > /home/dsl11/dev/llvm-upstream/src/lib/Transforms/Scalar/LoopStrengthReduce.cpp:950:5: > note: ...this statement, but the latter is misleadingly > > indented as if it is guarded by the ‘if’ > > NumIVMuls += isa<SCEVMulExpr>(Reg) && > > ^~~~~~~~~ > ```
Dan Gohman via llvm-dev
2016-Jun-03 17:31 UTC
[llvm-dev] -Wmisleading-indentation violations
In the LoopStrengthReduce.cpp code mentioned here, the code behaves as it was intended to; it's the indentation that's wrong. Dan On Thu, Jun 2, 2016 at 10:58 AM, Dan Liew <dan at su-root.co.uk> wrote:> Using the correct e-mail address this time. > > > > ``` > > > /home/dsl11/dev/llvm-upstream/src/lib/Transforms/Scalar/LoopStrengthReduce.cpp: > > In member function ‘void {anonymous}::Cost::RateRegister(const > > llvm::SCEV*, llvm::SmallPtrSetImpl<const llvm::SCEV*>&, const > > llvm::Loop*, llvm::ScalarEvolution&, llvm::DominatorTree&)’: > > > > > /home/dsl11/dev/llvm-upstream/src/lib/Transforms/Scalar/LoopStrengthReduce.cpp:943:3: > > warning: this ‘if’ clause does not guard... [-Wmisleading-indentation] > > > > if (!isa<SCEVUnknown>(Reg) && > > > > ^~ > > > > > /home/dsl11/dev/llvm-upstream/src/lib/Transforms/Scalar/LoopStrengthReduce.cpp:950:5: > > note: ...this statement, but the latter is misleadingly > > > > indented as if it is guarded by the ‘if’ > > > > NumIVMuls += isa<SCEVMulExpr>(Reg) && > > > > ^~~~~~~~~ > > ``` >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160603/e62fa90d/attachment-0001.html>