[PATCH] D11877: [Support] On Windows, generate PDF files for graphs and open with associated viewer

[Aaron Ballman via llvm-commits]

On Mon, Aug 17, 2015 at 12:43 PM, Michael Kruse <llvm at meinersbur.de> wrote:
> Thanks for the feedback.
>
> 2015-08-17 18:16 GMT+02:00 Aaron Ballman <aaron at aaronballman.com>:
>>> -  enum PSViewerKind { PSV_None, PSV_OSXOpen, PSV_XDGOpen, PSV_Ghostview };
>>> -  PSViewerKind PSViewer = PSV_None;
>>> +  enum ViewerKind {
>>> +    PSV_None,
>>> +    PSV_OSXOpen,
>>> +    PSV_XDGOpen,
>>> +    PSV_Ghostview,
>>> +    PDFV_CmdStart
>>
>> Why is this prefixed with PDFV_ instead of PSV_ like the rest of the
>> enumerators? Should have the same prefix since it's part of the same
>> enumeration.
>
> Because PS stands for "PostScript" while this will open a PDF viewer.
>
> However, considering that I renamed the enum's name as well, I can
> find a common prefix. ("V_" ?)

I would rename the enumerator prefixes to VK_ (for ViewerKind).

>
>
>>> +#ifdef LLVM_ON_WIN32
>>> +  if (!Viewer && S.TryFindProgram("cmd", ViewerPath)) {
>>> +    Viewer = PDFV_CmdStart;
>>> +  }
>>
>> Is there an advantage to using cmd instead of manually opening the
>> file with the related PDF viewer via ShellExecuteEx (or one of the
>> other Shell APIs)?
>
> The Support cross-platform layer doesn't expose such API (at least I
> am not aware of). Since this function is only useful at debug-time,
> calling cmd seems like a sufficient solution to me. I am not sure
> whether including Windows.h into this file will find general
> acceptance.

That's certainly fair. I'm just hesitant to spin up an entire command
processor and pass flags to it that aren't heavily vetted (I'm not
certain how much of this is under user control, so this is a bit of a
red flag to me). For instance, does this open up a way for this
feature to be abused to run arbitrary commands on a server-hosted
Clang (like coliru or wandbox)?

~Aaron

>
> Michael