shafik added a comment. In D139713#3989709 <https://reviews.llvm.org/D139713#3989709>, @Pierre-vh wrote:
> In D139713#3989071 <https://reviews.llvm.org/D139713#3989071>, @shafik wrote: > >> If I am reading the code correctly it looks like if the call to >> `(*I)->isValueDependent()` is true then the temporary will not be created >> and therefore we should not be attempting to access the slot. >> >> If this is the case then maybe the checking in >> `EvaluateWithSubstitution(...)` needs to be more carefully done? >> >> I am not familiar with this code but I don't know if you analysis provides a >> convincing case the assert should be removed. > > Indeed, this only happens when isValueDependent returns true. > > I am also not familiar with the code, so I just decided to propose a quick > fix to get the discussion started; I certainly don't mind changing the nature > of the fix if we agree it should be fixed differently. > For instance, we could also make the "getParam" call faillible by adding some > "tryGetParam" variant that doesn't have the assert, or by passing some > optional boolean to indicate it's acceptable to have the key present in the > map with a different version. > > My initial reasoning was that if the assert can be broken by legitimate code, > then it shouldn't be there It looks like the original commit that added `getTemporary(...)` which was 4e2698ca9e49e that this invariant held but the later commit which added `getParamSlot(...)` which is f7f2e4261a98b <https://reviews.llvm.org/rGf7f2e4261a98b2da519d58e7f6794b013cda7a4b> broke the invariant. So I think that looks like an error and any fix should maintain the invariant unless strongly motivated reasoning can be put forward to remove it but I am not totally sure. CC @rsmith @ahatanak who are the authors of the two commits mentioned above. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139713/new/ https://reviews.llvm.org/D139713 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
