[cfe-dev] [PATCH] Refactoring: fix making absolute FilePath in Replacement

[Steffen Prohaska]

On Sep 2, 2014, at 3:23 PM, Manuel Klimek <klimek at google.com> wrote:

> On Tue Sep 02 2014 at 2:38:59 PM Steffen Prohaska <prohaska at zib.de> wrote:
> 
>> ..., but it doesn't solve my original problem.
>> 
>> Maybe the following is relevant.  I'm unsure, because I'm rather
>> unfamiliar with the clang codebase.
>> 
>> I've been working on a custom refactoring tool.  I tried to save the
>> replacements and then apply them with clang-apply-replacements.  I
>> wanted to collect replacements for several packages across a large
>> project.  The problem can be illustrated with clang-modernize.
>> 
>> I created a compile_commands.json for a custom build system that uses
>> recursive make. Commands are executed in the subdirectory of the source
>> file:
>> 
>>     [
>>         {
>>             "directory": "/rootpath/src/package",
>>             "command": "clang++ ... file.cpp",
>>             "file": "file.cpp"
>>         },
>>         ...
>>     ]
>> 
>> I'd like to create serialized replacements similar to:
>> 
>>     clang-modernize --add-override --serialize-replacements \
>>         --serialize-dir=$(pwd)/replacements src/package/file.cpp
>> 
>> and then apply them with:
>> 
>>     clang-apply-replacements -format ./replacements
>> 
>> The setup seemed natural, and I expected it to work this way.  But it
>> fails, because the serialized replacements contain paths that are
>> relative to the directories in compile_commands.json (like
>> "FilePath: file.cpp").
>> 
>> With my fix that teaches Replacement to store absolute paths, it works
>> as expected, because the Replacements and therefore the YAML files then
>> contain absolute paths.
> 
> The problem is that this then leaves you with replacements that are not relocatable; for example, if you run the refactoring tool distributed over multiple machines, where each machine may have a random path component (machine dependent), you'll end up with replacements you cannot apply at all.

Well, you could not use them right away.  But it would be easy to
apply sed to remove the leading '/rootpath/' before collecting the
distributed files.  The resulting FilePaths would be relative to the
original working directory of clang-modernize.  Such paths seem ideal
to me.

Absolute paths might still be better than package-local paths, because
you can easily apply sed to all of them in one go, while you would need
to use per-subdirectory sed rules to get from package-local paths to
paths that are relative to the directory that clang-modernize had been
started in.


> Which format are you outputting replacements in?

I mimic clang-modernize YAML output:

    MainSourceFile:  ...
    Replacements:
      - FilePath:        ...
        Offset:          ...
        Length:          ...
        ReplacementText: ...


> Can you just call make_absolute on the replacement's path when you output them?

Yes.  This is what I do.  I map the instances of Replacement, so that
I can use the available YAML serialization:

    Replacement withAbspath(const Replacement& rep) {
        SmallString<200> abspath = rep.getFilePath();
        llvm::sys::fs::make_absolute(abspath);
        return Replacement(abspath, rep.getOffset(), rep.getLength(),
                           rep.getReplacementText());
    }

	Steffen