[PATCH] Refactoring: fix making absolute FilePath in Replacement

An std::error_code of 0 indicates success. The condition must be reversed.

See diff below for http://llvm.org/svn/llvm-project/cfe/trunk

Steffen Prohaska <prohaska@zib.de> writes:

An std::error_code of 0 indicates success. The condition must be reversed.

See diff below for http://llvm.org/svn/llvm-project/cfe/trunk

---

Hello,
I'm unsure whether this is the right way to submit a patch. Please let me
know if I should prepare it in a different way.

Generally, we prefer patches to be sent to cfe-commits@, rather than
cfe-dev@, for future reference.

Additionally, we usually like to check in a test with bug fixes. This is
pretty obvious by inspection, but if you have a case where it fails and
can easily be reduced, submitting that as well would be appreciated.

Justin Bogner <mail@justinbogner.com> writes:

Steffen Prohaska <prohaska@zib.de> writes:

An std::error_code of 0 indicates success. The condition must be reversed.

See diff below for http://llvm.org/svn/llvm-project/cfe/trunk>
---

Hello,
I'm unsure whether this is the right way to submit a patch. Please let me
know if I should prepare it in a different way.

Generally, we prefer patches to be sent to cfe-commits@, rather than
cfe-dev@, for future reference.

Additionally, we usually like to check in a test with bug fixes. This is
pretty obvious by inspection, but if you have a case where it fails and
can easily be reduced, submitting that as well would be appreciated.

On further inspection, fixing this opens a whole can of worms. It looks
like we've been relying on it being broken for quite a while. I've sent
an alternative patch to cfe-commits for review:

    http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20140901/113949.html

Hmm... Your patch looks sane, 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.

Best,
  Steffen

Justin Bogner <mail@justinbogner.com> writes:

Steffen Prohaska <prohaska@zib.de> writes:

An std::error_code of 0 indicates success. The condition must be reversed.

See diff below for http://llvm.org/svn/llvm-project/cfe/trunk

Hello,
I’m unsure whether this is the right way to submit a patch. Please let me
know if I should prepare it in a different way.

Generally, we prefer patches to be sent to cfe-commits@, rather than
cfe-dev@, for future reference.

Additionally, we usually like to check in a test with bug fixes. This is
pretty obvious by inspection, but if you have a case where it fails and
can easily be reduced, submitting that as well would be appreciated.

On further inspection, fixing this opens a whole can of worms. It looks
like we’ve been relying on it being broken for quite a while. I’ve sent
an alternative patch to cfe-commits for review:

http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20140901/113949.html

Hmm… Your patch looks sane, 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.

Which format are you outputting replacements in? Can you just call make_absolute on the replacement’s path when you output them?

..., 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