thr3ads.net - llvm dev - [llvm-dev] -Wmisleading-indentation violations [Jun 2016]

If this information is useful, please help other people find it:
Share via:

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

head link

[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

head link

[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

head link

[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

head link

[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

head link

[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

head link

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

Possibly Parallel Threads

Search for more possibly parallel threads

llvm dev - Jun 2016 - -Wmisleading-indentation violations

[llvm-dev] -Wmisleading-indentation violations

[llvm-dev] -Wmisleading-indentation violations

[llvm-dev] -Wmisleading-indentation violations

[llvm-dev] -Wmisleading-indentation violations

[llvm-dev] -Wmisleading-indentation violations

[llvm-dev] -Wmisleading-indentation violations

[llvm-dev] -Wmisleading-indentation violations

Possibly Parallel Threads