-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.

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?

/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

/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.

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.

Using the correct e-mail address this time.

In the LoopStrengthReduce.cpp code mentioned here, the code behaves as it was intended to; it’s the indentation that’s wrong.

Dan