[PATCH] cp11-migrate: Integration with LibFormat.
Guillaume Papin
guillaume.papin at epitech.eu
Sat Jul 20 13:09:25 PDT 2013
Thanks for the review. I reworked the code. I hope it's better now.
There is another patch that this patch depends on: http://llvm-reviews.chandlerc.com/D1190
================
Comment at: cpp11-migrate/Core/FileOverrides.h:132
@@ +131,3 @@
+ /// removed.
+ void collectReplacementData(const clang::tooling::Replacements &Replaces);
+
----------------
Edwin Vane wrote:
> Can this name be more specific to what it does: adjusting changed ranges.
Done, documentation changed as well.
================
Comment at: cpp11-migrate/Core/FileOverrides.h:136
@@ +135,3 @@
+ /// file.
+ void reformatFileChanges(llvm::StringRef Filename,
+ const clang::format::FormatStyle &Style,
----------------
Edwin Vane wrote:
> I'd suggest `reformatSingleFile()`.
Looks better. I managed to extract the Reformatting stuff to its own class, so it has been moved there.
================
Comment at: cpp11-migrate/Core/FileOverrides.cpp:104
@@ +103,3 @@
+
+/// \brief A functor owning a replacement, that says if a Range is
+/// "eaten" by the a replacement.
----------------
Edwin Vane wrote:
> The functor doesn't own the replacement.
I added the 'contains()' method to `tooling::Range` so the comment on this functor are no-longer applicable.
================
Comment at: cpp11-migrate/Core/FileOverrides.cpp:105
@@ +104,3 @@
+/// \brief A functor owning a replacement, that says if a Range is
+/// "eaten" by the a replacement.
+struct RangeContains {
----------------
Edwin Vane wrote:
> Can we be more precise? What's "eaten" mean? Let's have some post-conditions for this functor. Looking at the logic this functor might be better called "rangeContained" since we're testing if a replacement completely covers a range.
N/A anymore.
================
Comment at: cpp11-migrate/Core/FileOverrides.cpp:107
@@ +106,3 @@
+struct RangeContains {
+ const tooling::Replacement &R;
+ RangeContains(const tooling::Replacement &R) : R(R) {}
----------------
Edwin Vane wrote:
> Data members by convention go at the end of a record definition.
N/A anymore.
================
Comment at: cpp11-migrate/Core/FileOverrides.cpp:110
@@ +109,3 @@
+
+ bool operator()(tooling::Range D) const {
+ unsigned ReplacementEnd = R.getOffset() + R.getLength();
----------------
Edwin Vane wrote:
> Why D? Can we use some better variable names for R and D?
N/A anymore.
================
Comment at: cpp11-migrate/Core/FileOverrides.cpp:150
@@ +149,3 @@
+
+ FileChangesMap[R.getFilePath()].push_back(tooling::Range(Offset, Length));
+ }
----------------
Edwin Vane wrote:
> So what happens when a replacement and range partially overlap? Do you coalesce ranges or just end up with two overlapping ranges?
I rewrote the algorithm partially since it was erroneous (e.g: ranges partially replaced weren't truncated, meaning that they were longer than necessary and reformatting too much code for example).
The old algorithm allowed overlapping ranges but the new one coalesces the ranges.
The new algorithm is more thoroughly tested.
================
Comment at: cpp11-migrate/Core/FileOverrides.cpp:172
@@ +171,3 @@
+void SourceOverrides::reformatChanges(const clang::format::FormatStyle &Style) {
+ llvm::IntrusiveRefCntPtr<clang::DiagnosticOptions> DiagOpts(
+ new DiagnosticOptions());
----------------
Edwin Vane wrote:
> Any reason we're using the heap here? Since everything has scope local to this function can we just define them on the stack?
This is the 'pattern' present in Tooling/Format, they use this method (for the same scope as here).
I can check if you really want to see if it is really necessary or if everyone use the same wrong approach but it felt like a 'secure' way of doing it.
================
Comment at: cpp11-migrate/tool/Cpp11Migrate.cpp:151
@@ +150,3 @@
+ // Check the reformatting style option
+ llvm::OwningPtr<format::FormatStyle> ReformattingStyle;
+ if (FormatStyleOpt.getNumOccurrences() > 0) {
----------------
Edwin Vane wrote:
> Could we put this code in a helper function to help keep the clutter of main down?
Done. Note that this has changed a bit and now a `Reformatter` class is used instead of just the FormatStyle.
================
Comment at: cpp11-migrate/tool/Cpp11Migrate.cpp:224
@@ -185,1 +223,3 @@
+ if (ReformattingStyle)
+ for (FileOverrides::const_iterator I = FileStates.begin(),
----------------
Edwin Vane wrote:
> Why not use the variable TrackReplacements from above?
My reason is that, to reformat it is necessary to track changes but the opposite is not necessarily true (maybe there will be some other reasons in the future tu track changes). The bool TrackReplacements was created to make the call to the constructor more meaningful.
Also the code has changed a bit and now the OwningPtr is of type "Reformatter"
Pseudocode:
if (Reformatter)
Reformatter->reformat();
I read it as: "if we have a reformatter, reformat the files".
Let me know if you don't agree.
================
Comment at: docs/MigratorUsage.rst:71
@@ +70,3 @@
+
+ After all transformations have been applied it's possible to reformat the
+ changes using this option. The ``string`` arguments can be either a builtin
----------------
Edwin Vane wrote:
> its not it's (which is a contraction of "it is").
I think I mean "it is". Maybe a coma was missing but I don't think it changes the meaning:
"After all transformations have been applied, it is possible to..."
Anyway, I tried to reformulate this part to something more understandable.
================
Comment at: docs/MigratorUsage.rst:73
@@ +72,3 @@
+ changes using this option. The ``string`` arguments can be either a builtin
+ style, one of: LLVM, Google, Chromium, Mozilla or a YAML configuration file.
+ ClangFormat_ can generates the configuration file for with ``clang-format
----------------
Edwin Vane wrote:
> No :. Check out [[http://www.grammar-monster.com/lessons/semicolons_in_lists.htm|semi-colon use for lists]].
Hopefully it's fixed.
================
Comment at: docs/MigratorUsage.rst:74
@@ +73,3 @@
+ style, one of: LLVM, Google, Chromium, Mozilla or a YAML configuration file.
+ ClangFormat_ can generates the configuration file for with ``clang-format
+ -dump-config``.
----------------
Edwin Vane wrote:
> **generate** not **generates**. I'd suggest a rewordering: If you want a place to start for using your own custom configuration file, ClangFormat_ can generate a file with ...
I used your suggestion.
================
Comment at: docs/MigratorUsage.rst:91
@@ +90,3 @@
+ - MyMap.rbegin();
+ + auto I = MyMap.rbegin();
+
----------------
Edwin Vane wrote:
> Could you make the change made by -format-style more explicit here (i.e. examples with and without the option used).
Okay, I tried. Still 1 example but showing with and without the option this time. Let me know if you think more examples are needed.
================
Comment at: test/cpp11-migrate/Core/reformat.cpp:19
@@ +18,3 @@
+ std::auto_ptr<int> bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb;
+ // CHECK: std::unique_ptr<int> aaaaaaaa;
+ // CHECK-NEXT: std::unique_ptr<int>
----------------
Edwin Vane wrote:
> Will FileCheck still work if you add whitespace infront of the the CHECK so that the code after the : lines up?
Yep it works, it should be more readable now.
http://llvm-reviews.chandlerc.com/D1136
More information about the cfe-commits
mailing list