deconstructPathRelativeToSWD

[carmichaelong]

Addresses fixes to deconstructPathname (addresses issue #264). Introduces a new function deconstructPathRelativeToSWD that finds the absolute path to a given path name relative to a specified working directory (swd).

[tkuchida]

Are these tabs? 😱

[carmichaelong]

Oops. Yes. Many of them.

[sherm1]

Cool -- the build failed in 8 seconds due to Chris's hard core tab detector (see above). The message was: Tabs found in the lines shown above. See CONTRIBUTING.md about tabs.

[sherm1]

Do you know how to fix the tab problem, Carmichael?

[carmichaelong]

Nope (by the way, CONTRIBUTING.md is currently void of suggestions about tabs) . What's the best way to do that?

[chrisdembia]

See this part of the CONTRIBUTING.md. Maybe that section could be named differently, like "Do not use tabs for indenting; use 4 spaces".

[chrisdembia]

@sherm1 the CONTRIBUTING.md right now gives instructions for using spaces in the future, but doesn't give instructions for how to replace tabs after the fact.

[sherm1]

What's the best way to [get rid of tabs]?

Depends on the editor. In Visual Studio you can use Edit->Advanced->Untabify, or do it with Find & Replace with regular expressions on, using \t to represent a tab (replace them with 4 spaces). In vi or vim something like :1,$s/\t/ /g should do it.

Then after the tabs are gone, commit the changes, and then push to your deconstructPathRelativeToSWD branch again. GitHub will notice the change to this PR and new builds will get kicked off.

[chrisdembia]

@sherm1 I'm going to create a PR for CONTRIBUTING.md based on what you just said.

[sherm1]

I don't think you want to use getPathSeparatorChar() here, because that will return \ (backslash) on Windows, but you just turned all the slashes into forward slashes temporarily. So the last character won't ever match a backslash.

[chrisdembia]

The term "full absolute path" may be confusing. Perhaps just say "absolute path"? Once the isAbsolute boolean is renamed to something like isSearchable, "absolute path" won't be ambiguous anymore.

[carmichaelong]

I wasn't sure if "absolute path" becomes ambiguous after the boolean is renamed. To me, ./blah might still be considered an absolute path, but c:\blah\ would be the full path (I also considered just saying "full path").

[chrisdembia]

Ah okay. Maybe say "full path" and define what that means, with an example?

[sherm1]

I think "absolute path" is a good term but it needs to be defined. How about something like "by absolute path we mean a root-relative path name, and on Windows it will begin with an explicit drive letter." The term "full absolute path" doesn't have a well known meaning anyway so would also require a definition.

[chrisdembia]

It'd be nice if we could somehow simulate interesting values for these cwd's. I presume their values are probably x:\ and y:\.

[carmichaelong]

Agreed. I'm open to any ideas here. Even bringing in drives is causing issues with not-Windows platforms (see the fail on the last test), and any suggestions there would be nice. Things to think about:

1. What do we even want a non-Windows machine to return if a path has a drive letter?
2. How to do tests where we might/would expect different answers on different platforms?

[chrisdembia]

1. What do we even want a non-Windows machine to return if a path has a drive letter?

So I just checked, and on linux I can create a directory like /home/chris/C:/hello. So I think a drive letter on windows is treated just like a directory name, since that's possible (though, why would someone??). That is, on non-windows, treat C:/arbitrary/path as a relative path, just as you'd treat hello/world.

1. How to do tests where we might/would expect different answers on different platforms?

Maybe separate the tests by platform?

#ifdef _WIN32
    windows path tests
#else
    other path tests
#endif

See this code example.

If we wanted to account for the potential for people to cross-compile simbody (compile on windows for release on unix), then stuff gets really complicated.

[sherm1]

General comment: This routine is more complicated than I was imagining. I thought the strategy was going to be the following: Give swd and pathname:

• map the swd to an absolute path name by calling some other function that always uses the current working directory as the meaning of "."
• then use that if needed to resolve a relative pathname

Maybe that's what it is doing now but mixed into one method? Anyway, I think you want to start with a call to getAbsoluteDirectoryPathname(swd) which will convert a given swd to an absolute path and take care to make it terminate with a slash, also. So you don't need any of that code in the new method.

I believe that will cause a blank swd to be expanded to the current working directory (which is what you want it to do). (That needs checking to make sure.)

Is there any reason you can't use that method?

[sherm1]

I would like to see a less confusing name for this method; let's not continue in the tradition of bad names that I started when I named deconstructPathname() which should have been something more like determineWhetherToUseSearchPath() or something like that. How about something like: findAbsolutePathUsingSpecifiedWorkingDirectory(). To me that seems like the main purpose of the method; the deconstruction is kind of an aftereffect.

[carmichaelong]

I thought the plan for this method was that it would never call out to the system at all -- it was just going to be a string processing routine. Here I see several system calls to get the cwd and drive letter.

Anyway, I think you want to start with a call to getAbsoluteDirectoryPathname(swd) which will convert a given swd to an absolute path and take care to make it terminate with a slash, also. So you don't need any of that code in the new method.

I think you're right that most of these could go away with getAbsoluteDirectoryPathname(swd). One question is: If swd is the empty string, do we just want to return getAbsoluteDirectoryPathname(path)? I think so, and that would clean up the first bit.

I also think this could reduce any calls to the system to calling getCurrentDriveLetter() whenever swd or path does not provide a drive letter, which is currently true if swd is not the empty string.

[sherm1]

If swd is the empty string, do we just want to return getAbsoluteDirectoryPathname(path)?

That's a good idea, although it probably doesn't have to be treated as a special case as long as it works -- people shouldn't be calling this method with a blank swd normally! But if you want to do that, you should use getAbsolutePathname(path) because path doesn't have to be a directory.

[carmichaelong]

Another thought about what this function should do: Previously, getAbsolutePathname() called on deconstructPathname(). Do we want to remove the "deconstructing" part (i.e. separating it into a directory, fileName, and ext) and just return an absolute path and move the "deconstructing" part to new function?

[sherm1]

Do we want to remove the "deconstructing" part (i.e. separating it into a directory, fileName, and ext) and just return an absolute path and move the "deconstructing" part to new function?

I like that idea -- seems much cleaner. The only reason why not to do it would be if deconstructing has to get done anyway in order to construct the absolute path name, or if it extra cheap to do them both at the same time for some reason.

[carmichaelong]

The only reason why not to do it would be if deconstructing has to get done anyway in order to construct the absolute path name deconstructing has to get done anyway in order to construct the absolute path name

Well the way to make the canoncicalized path name is to deconstruct it then put it back together. I suppose that means it should be left as is (i.e. returning directory, fileName, and ext separately) or else you'd have to "deconstruct" again.

[carmichaelong]

@sherm1, ready to be looked at again.

[sherm1]

I think (2) actually is the first rule applied. If so should come first in this list.

[sherm1]

Done reviewing -- looks great, Carmichael. Ready except for the few minor comments above.

[carmichaelong]

Fixed the "//" problems. Also chose to do some rearranging of the logic to move some things that definitely could be called by "deconstructPathname()" earlier. It lengthens the code a little, so let me know if the logic is easier to follow. If not, the logic can be reverted if it's not worth it.

I'm also considering that a bit of code at the end could be removed by just calling on "deconstructPathname()" after figuring out how the paths are combined. Do you think it's worth it to remove some of that copied code?

[sherm1]

I'm also considering that a bit of code at the end could be removed by just calling on "deconstructPathname()" after figuring out how the paths are combined. Do you think it's worth it to remove some of that copied code?

Yes, definitely. In fact there are several cases where you can get rid of a bunch of code by using getAbsolutePathname() rather than deconstructPathname(). I think it already has the same 6 lines of code you are using.

[sherm1]

Oops -- never mind my last comment. I forgot you need the individual segments not just the absolute path.

[sherm1]

This doesn't make sense to me. The current drive would only be empty on a non-Windows system but then why would pathdrive not be empty?

[sherm1]

Currently I think there is no directly platform-dependent code in this method. Instead everything is hidden by platform-dependent methods that understand how to interpret the path characters so that "C:" just comes in as ordinary file characters on Linux. It would be good to keep it that way if possible. Testing whether getCurrentDriveLetter().empty() is really a hidden platform dependency; those should be done with #ifdef WIN32 and localized with platform dependencies elsewhere. In contrast testing pathdrive.empty() is not a platform dependency since it could be empty anywhere.

[sherm1]

I didn't notice any test cases with ".." segments in the pathnames. It would be good to have one or two of those just to make sure that features is still working right in the new methods.

[carmichaelong]

@sherm1, ready again. Has the changes you suggested and some directed tests at weirder cases.

[sherm1]

Sweet! I love it when the code gets shorter! Thanks so much for all this work, Carmichael. Merging now.