Page MenuHomePhabricator

[analyzer] JsonSupport: Escape escapes
ClosedPublic

Authored by Charusso on Jun 17 2019, 3:59 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Charusso created this revision.Jun 17 2019, 3:59 PM
NoQ added a comment.Jun 18 2019, 2:11 PM

Hmm, this doesn't seem to solve my problem in D62761. Let me write some actual test case so that you knew it's fixed when it's fixed.

In D63462#1549144, @NoQ wrote:

Hmm, this doesn't seem to solve my problem in D62761. Let me write some actual test case so that you knew it's fixed when it's fixed.

Well, I have did the same:

if (is_dot_node(line)):
        node_id = str_between(line, 'Node', ' [shape=record')
        label = get_label(line)
        label = (label.replace('\\"', '"').replace('\\\\', '\\') ...

I believe this is the correct behavior. What I have pointed out is the opposite: when we have a unicode character we have to escape it, so it would be that \\\\ case. Rewrite that to '\\' is cool and safe.

NoQ added a comment.Jun 18 2019, 2:56 PM

I mean, i'm removing backslashes but you're adding more backslashes. Therefore i think we're talking about different issues.

In D63462#1549225, @NoQ wrote:

I mean, i'm removing backslashes but you're adding more backslashes. Therefore i think we're talking about different issues.

You *have to* remove backslashes, because sometimes we have \\\\ so two of them packed together. Then when you remove it, it will be only one (\\ ), which is handled by JSON properly. Sadly it is very strict and cannot predict two should not be a problem.

NoQ added a comment.Jun 18 2019, 3:02 PM

My problem is demonstrated (and solved) by D63519. If i revert my changes but apply this patch instead, my test keeps failing.

In D63462#1549241, @NoQ wrote:

My problem is demonstrated (and solved) by D63519. If i revert my changes but apply this patch instead, my test keeps failing.

That is what we want. .replace('\\\\', '\\') \ is perfect, and should work with that patch as I use it as well.

NoQ added a comment.Jun 21 2019, 6:50 PM

I guess let's add a test for the unicode problem that you're seeing.

Charusso updated this revision to Diff 206248.Jun 24 2019, 9:58 AM
  • Test case added.
NoQ accepted this revision.Jun 24 2019, 7:24 PM

Thx!

I guess it makes sense to add a test into test/Analysis/exploded-graph-rewriter/escapes.c as well, so that to learn if we can actually parse it later.

This revision is now accepted and ready to land.Jun 24 2019, 7:24 PM
Charusso updated this revision to Diff 206356.Jun 24 2019, 7:56 PM
  • More test.
In D63462#1556831, @NoQ wrote:

Well, after a month of escaping we are still have problems, so it is truly comes to your brain.

Charusso updated this revision to Diff 206357.Jun 24 2019, 8:07 PM
  • A working one.
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 24 2019, 8:09 PM