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

[Tobias Grosser]

On 07/05/2011 11:51 AM, Joerg Sonnenberger wrote:
> On Tue, Jul 05, 2011 at 10:23:28AM -0500, Tobias Grosser wrote:
>> +  sys::Path prog;
>> +
>> +  // Set default grapher
>> +#if HAVE_CIRCO
>> +  prog = sys::Path(LLVM_PATH_CIRCO);
>> +#endif
>> +#if HAVE_TWOPI
>> +  prog = sys::Path(LLVM_PATH_TWOPI);
>> +#endif
>> +#if HAVE_NEATO
>> +  prog = sys::Path(LLVM_PATH_NEATO);
>> +#endif
>> +#if HAVE_FDP
>> +  prog = sys::Path(LLVM_PATH_FDP);
>> +#endif
>> +#if HAVE_DOT
>> +  prog = sys::Path(LLVM_PATH_DOT);
>> +#endif
>> +
>> +  // Find which program the user wants
>> +#if HAVE_DOT
>> +  if (program == GraphProgram::DOT)
>> +    prog = sys::Path(LLVM_PATH_DOT);
>> +#endif
>> +#if (HAVE_FDP)
>> +  if (program == GraphProgram::FDP)
>> +    prog = sys::Path(LLVM_PATH_FDP);
>> +#endif
>> +#if (HAVE_NEATO)
>> +  if (program == GraphProgram::NEATO)
>> +    prog = sys::Path(LLVM_PATH_NEATO);
>> +#endif
>> +#if (HAVE_TWOPI)
>> +  if (program == GraphProgram::TWOPI)
>> +    prog = sys::Path(LLVM_PATH_TWOPI);
>> +#endif
>> +#if (HAVE_CIRCO)
>> +  if (program == GraphProgram::CIRCO)
>> +    prog = sys::Path(LLVM_PATH_CIRCO);
>> +#endif
>
> This is assigning prog at least twice. This should be a switch on
> program and default to dot/fdp/neato/twopi/circo without full path if it
> couldn't be found during installation. I would even argue that no full
> path should be hard-coded without a good reason, but that's a separate
> change.

Hi Joerg,

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.

In general I fully agree with you that hardcoding paths is not a good 
idea. Making this change should be a lot easier, after the DisplayGraph 
function became a little bit more readable.

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 

}

Reducing/removing the use of absolute paths is not yet done. I agree 
this is something we want, however I have not yet thought enough about 
this to provide a robust patch.

Tobi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-GraphWriter-Use-switch-for-chain-of-conditions.patch
Type: text/x-diff
Size: 2773 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110705/85b45479/attachment.patch>