This revision was automatically updated to reflect the committed changes.
Closed by commit rL331049: Always normalize FileSpec paths. (authored by
gclayton, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D45977?vs=144021&id=144353#toc
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.
Looks fine to me. Normalization, at least as it is implemented now in
`remove_dots`, is a fairly heavy operation, so it makes sense to avoid it when
possible. And the extra speedup is great.
clayborg updated this revision to Diff 144021.
clayborg added a comment.
After doing performance tests, the code was 7 to 10 % slower if we didn't check
if a path needs normalization due to the llvm code making arrays of StringRef
objects and appending a path together. Restored and even improved
clayborg added a comment.
In https://reviews.llvm.org/D45977#1077719, @labath wrote:
> This code itself looks fine, I have just two minor comments.
>
> However, I do have a question about performance. I remember us being very
> worried about performance in the past, so we ended up putting in thi
> On Apr 25, 2018, at 12:54 AM, Pavel Labath via Phabricator
> wrote:
>
> labath added a comment.
>
> This code itself looks fine, I have just two minor comments.
>
> However, I do have a question about performance. I remember us being very
> worried about performance in the past, so we end
labath added a comment.
This code itself looks fine, I have just two minor comments.
However, I do have a question about performance. I remember us being very
worried about performance in the past, so we ended up putting in this like
r298876. This removes the normalization step during FileSpec
clayborg updated this revision to Diff 143801.
clayborg added a comment.
Added comment before llvm::sys::path_remove_dots(...)
https://reviews.llvm.org/D45977
Files:
include/lldb/Core/FileSpecList.h
include/lldb/Utility/FileSpec.h
source/Breakpoint/BreakpointResolverFileLine.cpp
source/
aprantl added inline comments.
Comment at: source/Utility/FileSpec.cpp:241
}
+ llvm::sys::path::remove_dots(resolved, true, LLVMPathSyntax(syntax));
`// Normalize the path by removing './' and other redundant components.`
Comment at: sour
clayborg updated this revision to Diff 143777.
clayborg added a comment.
Switch over to using llvm::sys::path::remove_dots(), remove the ::Normalize()
function and fix a few issue discovered during testing.
https://reviews.llvm.org/D45977
Files:
include/lldb/Core/FileSpecList.h
include/lld
labath added inline comments.
Comment at: source/Utility/FileSpec.cpp:406
+ m_directory.Clear();
+ }
}
clayborg wrote:
> labath wrote:
> > clayborg wrote:
> > > > they resolve to the same file in a virtual filesystem, where all
> > > > referenced path com
clayborg added inline comments.
Comment at: source/Utility/FileSpec.cpp:406
+ m_directory.Clear();
+ }
}
labath wrote:
> clayborg wrote:
> > > they resolve to the same file in a virtual filesystem, where all
> > > referenced path components exist and are
labath added inline comments.
Comment at: source/Utility/FileSpec.cpp:406
+ m_directory.Clear();
+ }
}
clayborg wrote:
> > they resolve to the same file in a virtual filesystem, where all referenced
> > path components exist and are directories
>
> Norma
clayborg added a comment.
I am trying to switch to using llvm::sys::path::remove_dots() and we will see
where we end up. By switching to this it means:
"./foo.c" --> "foo.c"
"./foo/bar.c" --> "foo/bar.c"
This makes is easier to see if a relative path matches another FileSpec since
we don't hav
labath added inline comments.
Comment at: source/Breakpoint/BreakpointResolverFileLine.cpp:130-131
+// we must leave the slash character though.
+while (relative_path.consume_front("."))
+ /* Do nothing. */;
+ }
What about paths like `.foo/bar.c` an
aprantl added a comment.
I'll take that back: make_absolute only copies when the path isn't already
absolute.
https://reviews.llvm.org/D45977
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/l
aprantl added a comment.
> Also not sure if the LLVM stuff will try to substitute in the current working
> directory for "."?
No it won't, remove_dots() has no side effects. There's a separate
make_absolute() function. That one will cause another copy. Personally, if I
have a choice between a
aprantl added inline comments.
Comment at: source/Utility/FileSpec.cpp:107
+ for (auto i = path.find('/'); i != llvm::StringRef::npos;
+ i = path.find('/', i + 1)) {
+const auto nextChar = safeCharAtIndex(path, i+1);
clayborg wrote:
> clayborg wrote:
>
clayborg added inline comments.
Comment at: source/Utility/FileSpec.cpp:107
+ for (auto i = path.find('/'); i != llvm::StringRef::npos;
+ i = path.find('/', i + 1)) {
+const auto nextChar = safeCharAtIndex(path, i+1);
clayborg wrote:
> aprantl wrote:
>
clayborg added inline comments.
Comment at: source/Utility/FileSpec.cpp:107
+ for (auto i = path.find('/'); i != llvm::StringRef::npos;
+ i = path.find('/', i + 1)) {
+const auto nextChar = safeCharAtIndex(path, i+1);
aprantl wrote:
> clayborg wrote:
>
aprantl added inline comments.
Comment at: source/Utility/FileSpec.cpp:107
+ for (auto i = path.find('/'); i != llvm::StringRef::npos;
+ i = path.find('/', i + 1)) {
+const auto nextChar = safeCharAtIndex(path, i+1);
clayborg wrote:
> I am fine switchi
clayborg added a comment.
In https://reviews.llvm.org/D45977#1076048, @aprantl wrote:
> One general question: why is this form of normalization preferred over
> calling realpath?
Normalization is everything we can do to fix up a path without knowing anything
about the current working director
clayborg added inline comments.
Comment at: source/Utility/FileSpec.cpp:107
+ for (auto i = path.find('/'); i != llvm::StringRef::npos;
+ i = path.find('/', i + 1)) {
+const auto nextChar = safeCharAtIndex(path, i+1);
I am fine switching to using the l
aprantl added a comment.
One general question: why is this form of normalization preferred over calling
realpath?
https://reviews.llvm.org/D45977
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listin
aprantl added inline comments.
Comment at: source/Utility/FileSpec.cpp:107
+ for (auto i = path.find('/'); i != llvm::StringRef::npos;
+ i = path.find('/', i + 1)) {
+const auto nextChar = safeCharAtIndex(path, i+1);
clayborg wrote:
> no as the only ca
clayborg updated this revision to Diff 143643.
clayborg added a comment.
Fix comment typo
https://reviews.llvm.org/D45977
Files:
include/lldb/Core/FileSpecList.h
include/lldb/Utility/FileSpec.h
source/Breakpoint/BreakpointResolverFileLine.cpp
source/Core/FileSpecList.cpp
source/Plugin
clayborg added inline comments.
Comment at: source/Utility/FileSpec.cpp:105
+//--
+bool needsNormalization(const llvm::StringRef &path) {
+ for (auto i = path.find('/'); i != llvm::StringRef::npos;
aprantl added inline comments.
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:189
+ // Always normalize our compile unit directory to get rid of redundant
+ // slashes and other path anonalies before we use it for path prepending
+ FileSpec local_spec(local_pa
clayborg updated this revision to Diff 143609.
clayborg added a comment.
Remove commented out code.
https://reviews.llvm.org/D45977
Files:
include/lldb/Core/FileSpecList.h
include/lldb/Utility/FileSpec.h
source/Breakpoint/BreakpointResolverFileLine.cpp
source/Core/FileSpecList.cpp
sou
aprantl added inline comments.
Comment at: lldb.xcodeproj/xcshareddata/xcschemes/darwin-debug.xcscheme:29
selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.GDB"
+ language = ""
shouldUseLaunchSchemeArgsEnv = "YES">
Is this an acci
clayborg updated this revision to Diff 143608.
clayborg added a comment.
Remove unwanted file changes.
https://reviews.llvm.org/D45977
Files:
include/lldb/Core/FileSpecList.h
include/lldb/Utility/FileSpec.h
source/Breakpoint/BreakpointResolverFileLine.cpp
source/Core/FileSpecList.cpp
clayborg created this revision.
clayborg added reviewers: labath, jingham, davide, pranavb.
Herald added subscribers: JDevlieghere, aprantl.
Always normalizing lldb_private::FileSpec paths will help us get a consistent
results from comparisons when setting breakpoints and when looking for source
31 matches
Mail list logo