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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
