[PATCH] D82092: [analyzer] Handle `\l` symbol in Windows specific file paths in exploded-graph-rewriter

Denys Petrov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Jun 19 04:17:18 PDT 2020


ASDenysPetrov marked an inline comment as done.
ASDenysPetrov added a comment.

In D82092#2100973 <https://reviews.llvm.org/D82092#2100973>, @vsavchenko wrote:

> Even though it doesn't seem that necessary, I would still vote to comply with `clang-format` in tests as well.


Yes, I just forgot to format it and was going to load it after.

> Additionally, I want to make a note that we are not really working hard on keeping all the scripts and the analyzer itself to be fully functional on Windows.

I think it depends on the platform you prefer, so that's natural. I also try to use Ubuntu on VM to check any differences.



================
Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:383
+            # when directory name starts with the letter `l`.
+            if sys.platform == 'win32':
+                # Find all `\l` (like `,\l`, `}\l`, `[\l`) except `\\l`,
----------------
NoQ wrote:
> NoQ wrote:
> > Behavior of this script should not depend on the host platform. Neither should it depend on the target platform if the graph is obtained via cross-compilation. It should only depend on the graph itself, as it's a simple dot file converter. The graph itself may, of course, depend on the target platform (maybe even on the host platform).
> > 
> > So it sounds like this fix should be on the C++ side.
> Like, i mean, i see no reason why it should be impossible to copy the original dot file from one machine to another and still be able to run the script. It's probably not a super-important use case but i don't see why would we consciously break that.
>So it sounds like this fix should be on the C++ side.
The first I did was C++ side. BTW an explicit literal `"string\\literal"` can also cause this issue.
But then I found that I am not sure what the solution could be. How can we let the script skip this particular case without modifiing the string? How can we modify the string to keep it logically valid? The only solution I see is to make the replacement selection more accurate. 

>Like, i mean, i see no reason why it should be impossible to copy the original dot file from one machine to another and still be able to run the script.
Correct! I'll keep the regex part for all platforms.

I've just checked the old code on Ubuntu using the sample. `char text[] = "string\\literal";` And it also fails as on Windows.
When I used the patch It passed. So this is not only a Windows related issue.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82092/new/

https://reviews.llvm.org/D82092





More information about the cfe-commits mailing list