Hope you didn't commit already. One of my comment is worth being corrected I
think, other are potential style issues.
================
Comment at: clang-modernize/Core/ReplacementHandling.h:109
@@ +108,3 @@
+ /// \param[out] Resulting name is placed here.
+ static void generateTempDir(llvm::SmallVectorImpl<char> &Result);
+
----------------
This could return a `std::string` to make the interface easier.
================
Comment at: clang-modernize/Core/ReplacementHandling.h:113-117
@@ +112,7 @@
+
+ llvm::SmallString<128> CARPath;
+ llvm::SmallString<128> DestinationDir;
+ bool DoFormat;
+ llvm::SmallString<8> FormatStyle;
+ llvm::SmallString<128> StyleConfigDir;
+};
----------------
I don't think it's wrong per se but I think using more than one
SmallString/Vector as class field should be avoided. Maybe this class will
never be used on the heap but it seems okay to me to have `std::string` here.
================
Comment at: clang-modernize/Core/ReplacementHandling.h:41
@@ +40,3 @@
+ /// \param[in] Dir Destination directory name
+ void setDestinationDir(const llvm::StringRef Dir) { DestinationDir = Dir; }
+
----------------
[personal_opinion]
I see a lot of const `llvm::StringRef`, IMHO StringRefs have the same "value
semantic" as primitive types. In the same way I don't use const for integers I
won't use it here.
[/personal_opinion]
================
Comment at: clang-modernize/Core/ReplacementHandling.cpp:32-33
@@ +31,4 @@
+
+ void *symbol = (void *)(intptr_t) & generateTempDir;
+ CARPath = fs::getMainExecutable(Argv0, symbol);
+ StringRef ParentPath = path::parent_path(CARPath);
----------------
Not sure but I think the space after `&` isn't needed.
Looking at `fs::getMainExecutable()` examples I saw the following that seems
easier to read:
```
static int staticSymbol;
std::string mainExecutable = llvm::sys::fs::getMainExecutable("oclint",
&staticSymbol);
```
================
Comment at: clang-modernize/Core/ReplacementHandling.cpp:34-35
@@ +33,4 @@
+ CARPath = fs::getMainExecutable(Argv0, symbol);
+ StringRef ParentPath = path::parent_path(CARPath);
+ SmallString<128> TestPath(ParentPath);
+ path::append(TestPath, "clang-apply-replacements");
----------------
Maybe not possible but seems like one line can be as clear as those 2 lines:
```
SmallString<128> TestPath = path::parent_path(CARPath);
```
================
Comment at: clang-modernize/Core/ReplacementHandling.cpp:147
@@ +146,3 @@
+ fs::createUniqueFile(Prefix + "_%%_%%_%%_%%_%%_%%.yaml", Result)) {
+ Error.append(EC.message().begin(), EC.message().end());
+ return false;
----------------
`error_code::message()` returns a new string object. Calling `begin()/end()`
will be from different objects potentially no?
http://llvm-reviews.chandlerc.com/D1836
BRANCH
use-car
ARCANIST PROJECT
clang-tools-extra
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
