[llvm-commits] [PATCH 2/6] Use a separate function to translate from dot to ps

[Tobias Grosser]

On 07/05/2011 03:11 PM, Joerg Sonnenberger wrote:
> On Tue, Jul 05, 2011 at 01:32:42PM -0500, Tobias Grosser wrote:
>> first of all, this is the existing code and the only thing I did was
>> to move it into a separate function such that this code is split
>> into smaller more understandable pieces. For this patch I would
>> prefer to not touch the existing code to make it very clear that the
>> comment "No functional change intended" actually holds.
>
> I don't believe much in shuffling code around and cleaning it up later.

I believe very much in small, evolutionary changes - especially if there 
are no test cases available and I do not know why people implemented 
stuff the way it is today.

The goal of these patches was to make the function more readable and to
uniformly provide non-blocking behavior for all graph viewers, but to 
otherwise keep the current behavior and to not introducing any bugs.

To achieve this, I tried to change as little as possible.

>> In respect of replacing this chain of conditions with a switch, As
>> stated before, I would prefer to not do this in this commit. I
>> attached a patch that could be committed in a subsequent commit.
>> This patch also does not change the behaviour, but changes only the
>> style. The resulting code will be:
>>
>> switch (program) {
>> #if HAVE_DOT
>>    case GraphProgram::DOT:   prog = sys::Path(LLVM_PATH_DOT);   break;
>> #endif
>> #if HAVE_FDP
>>    case GraphProgram::FDP:   prog = sys::Path(LLVM_PATH_FDP);   break;
>> #endif
>> #if HAVE_NEATO
>>    case GraphProgram::NEATO: prog = sys::Path(LLVM_PATH_NEATO); break;
>> #endif
>> #if HAVE_TWOPI
>>    case GraphProgram::TWOPI: prog = sys::Path(LLVM_PATH_TWOPI); break;
>> #endif
>> #if HAVE_CIRCO
>>    case GraphProgram::CIRCO: prog = sys::Path(LLVM_PATH_CIRCO); break;
>> #endif
>>    default:
>>
>> #if HAVE_DOT
>>      prog = sys::Path(LLVM_PATH_DOT);
>> #elif HAVE_FDP
>>      prog = sys::Path(LLVM_PATH_FDP);
>> #elif HAVE_NEATO
>>      prog = sys::Path(LLVM_PATH_NEATO);
>> #elif HAVE_TWOPI
>>      prog = sys::Path(LLVM_PATH_TWOPI);
>> #elif HAVE_CIRCO
>>      prog = sys::Path(LLVM_PATH_CIRCO);
>> #else
>>
>>      llvm_unreachable("No graph program available");
>> #endif
>>
>> }
>
> Go back a step and consider this code from the perspective of building
> binary packages. Why do I need to have graphviz installed at configure
> time of LLVM?

I do not know. I just know that it is like this today and there is no 
reason to change this, just because I want to provide a clean and 
consistent implementation for non-blocking F->viewCFG().
Those are orthogonal problems.

Even though these are orthogonal problems I support your idea of not 
relying on absolute paths. Yet, I would prefer if we could tackle this, 
after these cleanup patches are committed.

> switch (program) {
> case GraphProgram::DOT: prog = "dot"; break;
> case GraphProgram::FDP: prog = "fdp"; break;
> ...
> default:
>    llvm_unreachable("Invalid graph program");
> }

Your code has no fallback if one of the graphviz tools is
not available. This was available in the old code. As I personally do 
not use this part of the DisplayGraph code and do not know if someone 
else needs this, I would be afraid to remove this feature.

> If really necessary, make it possible to map the program to hard-coded
> path names, but I don't think that's very useful. In that case, it would
> certainly be much easier to specify the path to dot and use the -K
> option.

As said above, I do not know why people implemented this the way it is 
today and how people use this feature. I would strongly prefer to not 
change anything, where I do not fully understand the requirements.

If you are interested in triggering the absolute path problem, you may 
also want to look into the use of LLVM_PATH_GRAPHVIZ, LLVM_PATH_XDOT_PY, 
LLVM_PATH_GV and LLVM_PATH_DOTTY as well as the selection of the default 
graph viewer:

static GraphViewProgram::Name selectGraphViewer() { 

#if HAVE_GRAPHVIZ
   return GraphViewProgram::GRAPHVIZ;
#elif HAVE_XDOT_PY
   return GraphViewProgram::XDOT_PY;
#elif (HAVE_GV && (HAVE_DOT || HAVE_FDP || HAVE_NEATO || HAVE_TWOPI || \
                    HAVE_CIRCO))
   return GraphViewProgram::GV;
#elif HAVE_DOTTY
   return GraphViewProgram::DOTTY;
#else 

   return GraphViewProgram::NONE;
#endif 

}

We most probably need to check for each of the different viewers if its 
available in the path and if it is executable. Then we need to remove 
the detection of the absolute paths from autoconf and cmake. And we may 
want to think about a way to select the graph viewer in case there are 
several viewers available. The current way to only provide the path for 
the viewer you want will not work any more.

As these changes would probably increase this patchset quite a bit, I 
wonder if it is possible to commit my changes first and to trigger the 
absolute path problem afterwords.

Cheers
Tobi