Fix uninformative hp2ps error when the cmdline contains double quotes

Authored by watashi on Nov 27 2018, 4:34 PM.



Reapply D5346: Fix uninformative hp2ps error when the command args contains double quotes with fix incompatible shell quoting in tests. It seems
like $'string' is not recognized under all test environments, so let's
avoid it in tests.

Test Plan
hp2ps: "T15904".hp, line 2: integer must follow identifier

use new ghc and hp2ps to profile a simple program.

Diff Detail

rGHC Glasgow Haskell Compiler
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
watashi created this revision.Nov 27 2018, 4:34 PM
Harbormaster edited this Differential Revision.Nov 27 2018, 5:43 PM
tdammers requested changes to this revision.Dec 3 2018, 7:14 AM
tdammers added a subscriber: tdammers.
tdammers added inline comments.

It would be lovely if those whitespace changes could be kept separate from the actual functional changes; makes it a lot easier on reviewers, and also helps figure out merge conflicts down the road.


This moves the allocation before the ASSERT; this is potentially wasteful (though only cosmetically so), because when the assert doesn't hold, the memory will still have been allocated.


I found the old loop logic easier to follow than this. Either way, you have two variables that go into effective loop conditions (ch and i); the old version had ch as the principal loop condition (signalling the intent "loop until you find a double quote") and tracks i as a counter on the side; the new version has i as the principal loop condition (signalling the intent "keep incrementing a size counter forever"), and then side-aborts the loop based ch. I would say that the former is closer to the actual intent here, and the latter only adds confusion.

Is there a strong, compelling reason to write it like this?

This revision now requires changes to proceed.Dec 3 2018, 7:14 AM
watashi added inline comments.Dec 3 2018, 3:57 PM

@tdammers in this diff, we changed the loop breaking condition to support "s inside strings so we cannot use the original one.

bgamari accepted this revision.Dec 7 2018, 10:03 PM

Looks good to me.


I'm a bit confused: if the ASSERT fails then the program dies. Surely we don't care about an extra allocation in an unexpected failure path.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 11 2018, 5:21 PM
This revision was automatically updated to reflect the committed changes.