[PATCH] D17981: [clang-tidy] Fix clang-tidy to support parsing of assembly statements.

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

In http://reviews.llvm.org/D17981#382213, @etienneb wrote:

> In http://reviews.llvm.org/D17981#380350, @alexfh wrote:
>
> > Adding Manuel, who might have better ideas.
>


... and who is unavailable for the next couple of weeks, unfortunately. To unblock you, I'd suggest the following solution for now:

1. move this code to clang/lib/Tooling
2. gate the code and its dependencies on a CMake option (off by default)
3. commit it and explore alternative solutions
4. after Manuel comes back, discuss possible alternatives once again with a better understanding of possible alternatives

WDYT?

> > In http://reviews.llvm.org/D17981#374904, @rnk wrote:

> 

> > 

> 

> > > In http://reviews.llvm.org/D17981#374553, @etienneb wrote:

> 

> > >

> 

> > > > This is a huge difference. I didn't expect dependencies to bring so much code.

> 

> > > >  I'm not a fan of having an empty statement and increasing false positives ratio.

> 

> > > >  Would it be possible to skip whole declarations with asm-stm, and flag them as "ignored / not parsable"?

> 

> > >

> 

> > >

> 

> > > I don't actually think there are that many false positives, but I wanted to hear from Alex in case I'm wrong. I was hoping he had better ideas on how to suppress a diagnostic error in clang and run the clang-tidy checks anyway.

> 

> > 

> 

> > 

> 

> > I'm not familiar with the capabilities of MS-asm blocks, but if they can contain function calls, for example, we might lose valuable information, if we skip them. The possibility of a false positive depends on a specific check, it's hard to tell in general. There's also a more generic thing that can stop working properly: finding compilation errors inside asm blocks and applying fixes to these errors (if there are any fixes generated from parsing MS-asm blocks). Not sure how frequently this will happen though.

> 

> > 

> 

> > > My best idea is that we make this diagnostic a default-error warning and then build with -Wno-unparseable-assembly or something. That's not a very good solution, though. =\

> 

> > 

> 

> > 

> 

> > Yes, not a very good solution for the reasons outlined above.

> 

> > 

> 

> > > 

> 

> > 

> 

> > > 

> 

> > 

> 

> > > > We could gate this code under a define. I'm not a fan of define, but it seems to be a compromise for the size.

> 

> > 

> 

> > > 

> 

> > 

> 

> > > > 

> 

> > 

> 

> > > 

> 

> > 

> 

> > > > Something like: LIBTOOLING_ENABLE_INLINE_ASM_PARSER

> 

> > 

> 

> > > 

> 

> > 

> 

> > > > 

> 

> > 

> 

> > > 

> 

> > 

> 

> > > > If we decide to pursue that direction, then it should probably be for every tools.

> 

> > 

> 

> > > 

> 

> > 

> 

> > > 

> 

> > 

> 

> > > I'd really rather not do that.

> 

> > 

> 

> > 

> 

> > What's your concern? If we want parsing code inside MS asm blocks like the compiler does, we'll need to pay the cost of the asm parser. If the binary size is a large problem here, can we maybe reduce the set of dependencies of the MS asm parser?

> 

> > 

> 

> > In any case, I'm sure many users don't need this functionality, so it should be made compile-time configurable.

> 

> 

> The alternative was to plug a dummy asm parser in clang-tidy, to mock the real one in the back-end.

>  I think it's doable, but didn't investigate it.


It would be useful to know whether it's possible with a reasonable effort and what the implications will be.

> Or we can add a fake browser to avoid paying the cost of the full one and all the dependencies.


I'm not sure I understand what you mean by "fake browser".

> Both direction make sense, and I will let you choose:

> 

> - Adding the real parser and paying the cost

> - Finding a way to parse and ignore any errors related to assembly statement

> 

>   If we choose to add the expensive dependencies, then we need to choose to gate it or not under a "define".


If we choose to add these dependencies, we should definitely gate them on a CMake option.

> Also, a point to bring here: this is applicable to every tool built with libtooling.


Yes, so it at least makes sense to move the code to clang/lib/Tooling.


http://reviews.llvm.org/D17981