Skip to content

configuration parser#2016

Merged
kinke merged 24 commits intoldc-developers:masterfrom
rtbo:conf_parser
Mar 18, 2017
Merged

configuration parser#2016
kinke merged 24 commits intoldc-developers:masterfrom
rtbo:conf_parser

Conversation

@rtbo
Copy link
Contributor

@rtbo rtbo commented Feb 25, 2017

Hi guys,

Following #1832 and #2007, yet another config file pull request.
Hopefully the last one!

.travis.yml Outdated
install:
- if [ "${TRAVIS_OS_NAME}" = "linux" ]; then export CC="gcc-4.9"; export CXX="g++-4.9"; fi
- if [ "${TRAVIS_OS_NAME}" = "osx" ]; then brew update; brew install libconfig; fi;
- if [ "${TRAVIS_OS_NAME}" = "osx" ]; then brew update; fi;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can delete the whole line, then.

@kinke
Copy link
Member

kinke commented Feb 25, 2017

I like it, thanks. Some unittests wouldn't hurt; I guess we'll have to switch from reading directly from file to reading the full config string then.

@rtbo
Copy link
Contributor Author

rtbo commented Feb 25, 2017

Some unittests wouldn't hurt

I Agree. I don't think any build target exists yet to run unittest blocks in the ldc executable.

@JohanEngelen
Copy link
Member

run unittest blocks in the ldc executable.

I am very strongly against adding unittests inside the source. It drowns code and promotes only shallow unittesting. Adding a general unittest framework to our repo would be nice, but for now perhaps you can just add some lit tests with %ldc -conf=... .

@dnadlinger
Copy link
Member

dnadlinger commented Feb 25, 2017

I am very strongly against adding unittests inside the source. It drowns code and promotes only shallow unittesting. Adding a general unittest framework to our repo would be nice, but for now perhaps you can just add some lit tests with %ldc -conf=... .

A separate unittest executable and using the normal D facilities would be just fine.

@kinke
Copy link
Member

kinke commented Feb 26, 2017

I like the separate LDC unittest executable. As building it only recompiles the D modules, it doesn't even take a minute on our CI systems.

@kinke
Copy link
Member

kinke commented Mar 3, 2017

Any remaining concerns?

This was referenced Mar 5, 2017
@JohanEngelen
Copy link
Member

It'd be good to get this in 1.2-alpha too

@dnadlinger
Copy link
Member

dnadlinger commented Mar 6, 2017

We can probably overhaul the code to make use of Phobos in the future, but it seems like everything should be okay for now.

@kinke
Copy link
Member

kinke commented Mar 6, 2017

It'd be good to get this in 1.2-alpha too

I don't think so. See #1960 (comment).

@dnadlinger
Copy link
Member

I don't think so

Ideally, this wouldn't affect users at all due to compatibility with the old implementation.

@kinke
Copy link
Member

kinke commented Mar 6, 2017

It doesn't. But it gets rid of the libconfig runtime dependency and as such distro maintainers should get rid of it too. In order not to annoy them with changes in 1.2 and 1.3 and to accelerate the 1.2 release (didn't we say one month ago that we're going to put out an alpha/beta immediately?), I'd prefer to delay it to 1.3, whose release cycle can theoretically (very theoretically given the very limited current manpower) be started these days anyway now that merge-2.073 is green too.

@kinke kinke mentioned this pull request Mar 18, 2017
@kinke kinke merged commit 1631bfb into ldc-developers:master Mar 18, 2017
@rtbo rtbo deleted the conf_parser branch May 7, 2017 22:06
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.

4 participants