Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimized suffix checking. #47

Merged
merged 1 commit into from
Mar 16, 2013
Merged

Optimized suffix checking. #47

merged 1 commit into from
Mar 16, 2013

Conversation

eskimor
Copy link

@eskimor eskimor commented Mar 15, 2013

Suffix checking easier and more efficient.

I also fixed some unittests some others that triggered, I disabled (Soenke please review).

My next pull request will most likely fix the hard coded location of app.d found in some places. I would also use the opportunity and make it configurable via an appFile setting.
So afterwards it will use appFile (if defined, platform specific suffix will be supported), if none is specified it will use app.d or projectName.d when found, just as it does now. Would this be ok? Is the name appFile ok?

Suffix checking easier and more efficient.

I also fixed some unittests some others that triggered I disabled (Soenke please review).
@eskimor
Copy link
Author

eskimor commented Mar 15, 2013

Should I change the matchesSpecification() method to accept the suffix without the leading hyphen? This way some string concatenations could be avoided. If yes, should it also accept it with the leading hyphen?

@s-ludwig
Copy link
Member

Since performance/memory allocations is a non-issue for dub, I wouldn't prioritize it. It if is done though, I would tend to let it accept both ways explicitly (using a bool parameter) to make sure that it doesn't accept invalid things ("-", "libswindows", "platforms": ["-linux"]).

Regarding the app.d problematic, I feel we need to start a more thorough discussion first. As you have probably seen, that "appFile" field would basically be equivalent to adding that file as an "excludedFiles" setting in all non-application configurations, except when --rdmd is used. And --rdmd is still not working when bulding a static or dynamic lib, so that's a related problem ("mainFile"/"rootFile"?). And then there is still the thing with vibe.d's -version=VibeCustomMain that I would like to improve if possible and that may also interact with the descision how to handle the app file. Let's open a forum topic, so it doesn't get lost here.

@eskimor
Copy link
Author

eskimor commented Mar 16, 2013

The thought was just, leaving out the leading '-' is trivial when calling the method, adding it is more work. That's why I thought not requiring it might be good. I would not go so far as to add a boolean parameter, because doing suffix[1..$] is trivial enough, changing the suffix extraction code to drop the leading hyphen in the first place too.

Ok, we are not going to accept both, if you have no hard feelings either way I would change it to not accept a leading hyphen.

@s-ludwig
Copy link
Member

Sounds good!

@eskimor
Copy link
Author

eskimor commented Mar 16, 2013

But it isn't :-) Because you have to special case the empty suffix. (in toJSON for example). Also the suffix[1..$] does not work for the empty suffix. I'll leave it as is.

@eskimor
Copy link
Author

eskimor commented Mar 16, 2013

Btw. I already started a discussion in the forum about the app.d file.

@s-ludwig
Copy link
Member

Re: special case: That's also what I had in mind with the boolean parameter - so not too much of a problem to do but even less of a problem in the first place ;)

I'll merge and try to collect the point that I see WRT the app.d topic later.

s-ludwig added a commit that referenced this pull request Mar 16, 2013
Optimized suffix checking.
@s-ludwig s-ludwig merged commit feb91e7 into dlang:master Mar 16, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants