ABataev added a comment. In D71241#1778564 <https://reviews.llvm.org/D71241#1778564>, @hfinkel wrote:
> In D71241#1778134 <https://reviews.llvm.org/D71241#1778134>, @ABataev wrote: > > > > > > ... > > >> > >> > >>> Also, check how -ast-print works with your solution. It returns different > >>> result than expected because you're transform the code too early. It is > >>> incorrect behavior. > >> > >> This is debatable. AST print does not print the input but the AST, thus > >> what is correct wrt. OpenMP declare variant is nowhere defined but by us. > >> Arguably, having it print the actually called function and not the base > >> function is preferable. Thus, the new way is actually more informative. > > > > You're completely wrong here! We shall keep the original AST. This is used > > in several tools and you significantly modify the user code. I consider it > > a real issue here. > > Alexey, again, this kind of comment is not appropriate. We're all experienced > developers here, and we all understand the importance of tooling support in > Clang. We also serve developers who write tools using AST matchers and other > Clang analysis facilities. Having the resolved callee represented in the AST > for what looks like a static call from the base-language perspective makes a > lot of sense from a tooling perspective. When performing static analysis on > the code, forcing a tool to understand how OpenMP variant selectors work in > order to perform inter-procedural static analysis is suboptimal in nearly all > cases. It is also true that we might want the base callee represented in some > way, but as that callee is never actually called, and one of the variants is > always called at that call site, it is important that IPA propagate > information into and out of the correct callee in order to produce the > correct results. If we currently represent the base callee as the callee that > will appear in the call graph, that's a bug: Clang's static analyzer will > produce incorrect results. > > If you know of specific tools that indeed depend on the current behavior to > produce correct results, please provide us with details on what they're doing > so that we understand the use cases. Regardless, we should prioritize > correct-by-default functioning of AST-based call graphs and their associated > static analysis. 1. This is significant issue that you're modifiy the original user code. 2. It may have some troubles with serialization/deserialization probably. Plus, I just don't understand why it is good to rewrite the code for the user but to keep the original code is bad. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71241/new/ https://reviews.llvm.org/D71241 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
