Fix uninformative hp2ps error when the cmdline contains double quotes
ClosedPublic

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

Details

Summary

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

Repository
rGHC Glasgow Haskell Compiler
Branch
hp2ps_job_parse (branched from master)
Lint
Lint WarningsExcuse: .
SeverityLocationCodeMessage
Warningutils/hp2ps/HpFile.c:183TXT3Line Too Long
Warningutils/hp2ps/HpFile.c:194TXT3Line Too Long
Unit
No Unit Test Coverage
Build Status
Buildable 25305
Build 64047: [GHC] Linux/amd64: Continuous Integration
Build 64046: [GHC] OSX/amd64: Continuous Integration
Build 64045: [GHC] Windows/amd64: Continuous Integration
Build 64044: arc lint + arc unit
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.
utils/hp2ps/HpFile.c
18

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.

410

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.

416

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
utils/hp2ps/HpFile.c
424–430

@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.

utils/hp2ps/HpFile.c
410

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.