================
Comment at: cpp11-migrate/Core/IncludeExcludeInfo.cpp:53
@@ +52,3 @@
+/// path i.e. "..", ".".
+void RemoveRelativePath(SmallString<64>& AbsPath) {
+ sys::path::const_iterator PathI = sys::path::begin(AbsPath);
----------------
Guillaume Papin wrote:
> Taking a StringRef as argument and returning a string seems more natural to
> me.
>
> If this is really needed in terms of performance maybe I will use something
> like http://llvm.org/docs/doxygen/html/Path_8cpp_source.html#l00696 (taking a
> SmallVectorImpl).
>
I agree, I don't think performance will be an issue in this case. I'll use
StringRef.
================
Comment at: cpp11-migrate/Core/IncludeExcludeInfo.cpp:56
@@ +55,3 @@
+ sys::path::const_iterator PathE = sys::path::end(AbsPath);
+ std::vector<StringRef> NewPath;
+ while (PathI != PathE) {
----------------
Guillaume Papin wrote:
> Why not SmallVector?
done
================
Comment at: cpp11-migrate/Core/IncludeExcludeInfo.cpp:59
@@ +58,3 @@
+ if (PathI->equals("..")) {
+ NewPath.pop_back();
+ // If this codition is reached then the given path is incorrect.
----------------
Guillaume Papin wrote:
> Shouldn't you check the stack size before doing a pop_back() instead?
>
yes, I got this one in the new patch
================
Comment at: cpp11-migrate/Core/IncludeExcludeInfo.cpp:62
@@ +61,3 @@
+ if (NewPath.size() == 0) {
+ llvm::errs() << "Incorrect path provided: " << AbsPath << "\n";
+ AbsPath = "";
----------------
Guillaume Papin wrote:
> Does this error message helps the user?
> Maybe if we are sure make_absolute is called beforehand change this to an
> assert? Unless make_absolute can produce some "wrong" paths.
we can't rely on make_absolute since the user can specify ../../.. till the
root is reached. We could silently return an empty path and remove it from the
list but I think it's better if at least we provide some feedback.
================
Comment at: cpp11-migrate/Core/IncludeExcludeInfo.cpp:72-73
@@ +71,4 @@
+ AbsPath = "";
+ for (std::vector<StringRef>::iterator I = NewPath.begin(),
+ E = NewPath.end();
+ I != E; ++I) {
----------------
Guillaume Papin wrote:
> Is it Phabricator or the indentation is incorrect here?
already fixed
================
Comment at: cpp11-migrate/Core/IncludeExcludeInfo.cpp:95
@@ -70,1 +94,3 @@
+ // Add only non-empty paths to the list.
+ if (Path.size() > 0)
List.push_back(Path.str());
----------------
Guillaume Papin wrote:
> Path.empty()
done
http://llvm-reviews.chandlerc.com/D1134
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
