aaron.ballman added a comment. In D131314#3710514 <https://reviews.llvm.org/D131314#3710514>, @inclyc wrote:
> In D131314#3710331 <https://reviews.llvm.org/D131314#3710331>, @aaron.ballman > wrote: > >> In D131314#3707131 <https://reviews.llvm.org/D131314#3707131>, @inclyc wrote: >> >>> ping >> >> FWIW, we usually only ping a review that hasn't had any activity in a week >> or more (it's not uncommon for reviews to sit for a few days while people >> think about them or reviewers are busy on other stuff). >> >> I've not had the chance to do an in-depth review yet, but I have two >> thoughts I can share early on. I think you can simplify the patch a little >> bit by treating a string literal and an initializer list the same way (using >> an iterator to walk over the elements) instead of ginning up a fake >> `StringLiteral` AST node (that's a very heavy handed way to implement this). >> However, even with that simplification, I'm not certain the use cases for >> the diagnostic happen enough to warrant this large of a change in how we >> process format strings. I had encouraged such a diagnostic given the >> equivalence of the construct with string literals, but I was imagining that >> the support would be a few lines of code rather than anything substantial >> like this. Perhaps you can find a way to make the changes less invasive, but >> if not, I think we may want to hold off on this change until a user files an >> issue pointing out some real world code that would be easier to fix if they >> had such a diagnostic. > > Clang seems to depend class `FormatStringLiteral` when performing format > string checking, which has its `SourceLocation`, a wrapped pointer. With > these information we can generate FixIt Hints directly on source codes. In > previous version, we probably haven't even considered checking the format > string evaluated from `InitListExpr` (just consider it is not a string > literal). I've tried adding some ways to treat them as equals before, but it > seems more invasive than this patch. Actually, I don't think any user would > ever define a format string like `{'%', 'd', 0}` (That's exactly the reason I > split format string checks in two patches). So maybe we should abandon this > patch because there are no enough benefits to apply this? I think that might be the best course of action, though I'm really sorry to have asked you to do this work only to turn around and say "nevermind." I just don't see enough users writing their format strings that way for the amount of effort it takes to support diagnosing it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131314/new/ https://reviews.llvm.org/D131314 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
