"I would just change it to GetLastPathComponent() and and leave GetDirectory() alone
or change it to GetContainingDirectory()."
Greg's suggestion maps very cleanly onto DWARF debug information, since
with that we a compilation unit directory, e.g.
/home/mg11/src/projects/browser
and then the notion of paths relative to that compile directory:
main.cpp
server/socket.cpp
server/script_parse.cpp
utils/logger.cpp
if I recall correctly...
Matt
Greg Clayton wrote:
On Aug 26, 2014, at 12:39 PM, Zachary Turner <ztur...@google.com> wrote:
I guess I'm thinking of this as the standard unix basename / dirname paradigm.
Everyone understands this paradigm and there's no confusion about it. And
given that FileSpec already internally stores its components in this format, it
would be a natural fit for m_filename and m_dirname to correspond to basename
and dirname, respectively. In fact, the entire class is already implemented
with this assumption. See, for example, SetFile() which can be used to set
arbitrary paths, which could refer to directories, but which still splits the
last component and stores it in m_filename. Or AppendPathComponent(), which
handles the case where m_filename is not null, hence assuming that it must
refer to a directory.
As for the places that broke as a result of my last change - certainly they need to be
fixed, so "reverting to green" is reasonable until we can decide if there's a
better fix. However, I did run all tests, and none of them failed. I don't think the
new test which checks that GetFilename() is null is particularly useful, because it just
tests an implementation detail of the class, and not a documented behavior of the class.
It should be possible to write a test against whatever failed, which probably would be a
better test.
There was no test and thus why I added one. But I do agree that the API is not
consistent.
In any case, back to my original question: Is there any object to changing
FileSpec::GetDirectory() and FileSpec::GetFilename() to be GetBaseName() and
GetDirName()?
I would just change it to GetLastPathComponent() and and leave GetDirectory()
alone or change it to GetContainingDirectory().
This would require changing the call sites of the places who previously broke
as a result of my original change. I don't actually know what broke since my
test runs were all successful, though.
The other issue with changing things is that we have internal branches that are
based on this code that are constantly requiring merging due to all of these
changes that have been going on. I appreciate all the changes, and changing
things to be better organized is nice.
If this is not appropriate, then I think we need to change
AppendPathComponent(), SetFile(), and various other functions to stop allowing
situations where m_directory + m_filename refers to a directory.
A few core design things:
- no setters can require any stat calls to determine if things are directories
unless someone explicitly asks the FileSpec for this info
- basename and directory must remain separate ConstString values
The Host calls that got the directories were optimized for the use cases where
client asks for directory X and then sets the basename via the GetFilename()
accessor. I would prefer to maintain this if possible to avoid contaminating
the constant string pool and we can do this via APIs.
Greg
_______________________________________________
lldb-commits mailing list
lldb-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
To report this email as spam click
https://www.mailcontrol.com/sr/MZbqvYs5QwJvpeaetUwhCQ== .
Member of the CSR plc group of companies. CSR plc registered in England and
Wales, registered number 4187346, registered office Churchill House, Cambridge
Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Keep up to date with CSR on our
technical blog, www.csr.com/blog, CSR people blog, www.csr.com/people, YouTube,
www.youtube.com/user/CSRplc, Facebook,
www.facebook.com/pages/CSR/191038434253534, or follow us on Twitter at
www.twitter.com/CSR_plc.
New for 2014, you can now access the wide range of products powered by aptX at
www.aptx.com.
_______________________________________________
lldb-commits mailing list
lldb-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
