aaron.ballman added a comment.

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.


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

Reply via email to