[llvm-commits] [llvm] r52288 - in /llvm/trunk/lib/System: Path.cpp Unix/Path.inc Win32/Path.inc

[Ted Kremenek]

On Jun 16, 2008, at 1:23 AM, Matthijs Kooijman wrote:

> Hi Argiris,
>
>
>> There are a couple of others that are also duplicated, like
>> Path::getBasename().
> IMHO code duplication should be prevented as much as possible. You  
> probably
> did well to make things consistent in this case, but perhaps we  
> should wonder
> if we shouldn't prevent all of this duplication.
>
>> I assumed that the convention is that if a method deals with  
>> directory
>> separators, it is getting into platform implementation details and  
>> it should
>> have separate platform implementations, even if the code turns out  
>> to be
>> identical (currently the Win32 path implementation uses '/' as  
>> directory
>> separator but it could as easily use '\' instead).
> Wouldn't it be better to simply make the path separator configurable  
> (ie, have
> a method that returns it?)
>
> Anyone with a better view of why things are like this that can  
> comment?

I agree with Matthijs: I don't think there is any good reason for the  
code duplication.

The duplication is probably just there because of laziness (I admit  
that I'm probably one of the culprits).  We should definitely refactor  
the common parts so that the common code gets exercised, tested, and  
regularly maintained on more machines.