ioeric added a comment.

Let me know when broken tests are fixed and this patch (and the corresponding 
patch) is ready again for review. Also let me know if you need any help.



================
Comment at: change-namespace/ChangeNamespace.cpp:892
+      llvm::errs() << llvm::toString(Style.takeError()) << "\n";
+      continue;
+    }
----------------
amaiorano wrote:
> amaiorano wrote:
> > amaiorano wrote:
> > > ioeric wrote:
> > > > I'd still like to apply replacements that are not cleaned up. Could you 
> > > > add the following line before continue, thanks! :)
> > > > ```
> > > > FileToReplacements[FilePath] = Replaces;
> > > > ```
> > > Will do.
> > Ah darn, just realized that I forgot to make this change you asked for 
> > @ioeric. Let me know if everything else is okay, and I'll be sure to add 
> > this in before submitting. 
> @ioeric Was looking at this code, and I'm wondering about the change you 
> wanted me to make here. You said that you'd still like to apply replacements 
> that are not cleaned up, so shouldn't that also be the case for when 
> format::cleanupAroundReplacements fails (just below)? In other words, 
> shouldn't it be:
> 
> ```
>     // Clean up old namespaces if there is nothing in it after moving.
>     auto CleanReplacements =
>         format::cleanupAroundReplacements(Code, Replaces, *Style);
>     if (!CleanReplacements) {
>       llvm::errs() << llvm::toString(CleanReplacements.takeError()) << "\n";
>       FileToReplacements[FilePath] = Replaces;
>       continue;
>     }
>     FileToReplacements[FilePath] = *CleanReplacements;
>   }
> ```
> 
> If so, I propose instead that we add 
> ```
> FileToReplacements[FilePath] = Replaces;
> ``` 
> before the call to format::getStyle so that if either of the format-related 
> calls below it (getStyle or cleanupAroundReplacements) fail, we get the 
> unclean replacements.
After taking a closer look, I realize that we don't actually need this change 
since `Replaces` is a reference to `FileToReplacements[FilePath]`.


https://reviews.llvm.org/D28315



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to