Szelethus added a comment. In D62883#1545343 <https://reviews.llvm.org/D62883#1545343>, @Charusso wrote:
> In D62883#1545341 <https://reviews.llvm.org/D62883#1545341>, @Szelethus wrote: > > > In D62883#1545324 <https://reviews.llvm.org/D62883#1545324>, @Charusso > > wrote: > > > > > As @NoQ pointed out we have some problem with that function. We are > > > tracking *values* without using the `redecl()`-chain (see in > > > https://clang.llvm.org/doxygen/DeclBase_8h_source.html#l00948), or > > > without tracking conditions. You have solved(?) the latter, but the > > > former is equally important to solve. > > > > > > I'm a little confused, which is the "former" and "latter" you're referring > > to? > > > I believe you have solved the condition tracking as you move in-place what is > going on. I'm still not sure I follow :) What are the two distinct things to be solved here? >>> I believe this patch should be on by default, but it requires to fix the >>> "In which range do we track?" problem with D62978 >>> <https://reviews.llvm.org/D62978>. >> >> I disagree with this. The reason why I'd like to make this an off-by-default >> option is to implement my followup improvements incrementally (not only >> that, but a whole family of conditions is going to be added), and allow us >> to observe the changes those make in relation to this patch -- besides, I >> don't really see this patch changing even if I manage to fix those issues. >> Would you like to see a change being made to this specific patch? > > As you moving stuff around it just bypasses the usefulness of the mentioned > two visitors, which is a problem. Also you have mentioned some very crazy bad > side-effects which is yes, related to in which range do we operate. Could you please elaborate? The particular thing I'm missing, I guess, is the need to get those issues fixed before this patch, and making this on-by-default. If anything, enabling a flag like this could really demonstrate changes on those problems. I also like to think that tracking a condition (especially a condition of a control dependency of yet another condition) is a drastically different case that tracking a "regular" variable (in particular, I think more aggressive pruning could be used), and such a change would definitely be out of scope for this patch. I like to think that most of the unimportant notes can be easily categorized, as seen above. With some, admittedly crude heuristics, we could cut these down greatly, and leave some breathing room for fine-tuning it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62883/new/ https://reviews.llvm.org/D62883 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
