Page MenuHomePhabricator

[libcxx] Refactor lit.cfg.
ClosedPublic

Authored by danalbert on Aug 18 2014, 1:08 AM.

Details

Summary

The purely imperative format of the configuration performed in lit.cfg was making merge conflicts with changes I have for Android an unbelievable pain in the ass. I've moved all of the configuration into a Configuration class, with each piece of configuration happening in a different method. This way I can avoid merge conflicts, and any new features that get added (as have been done with the sanitizers, the -std flag, etc.) can be easily applied to Android as well.

Diff Detail

Event Timeline

danalbert updated this revision to Diff 12611.Aug 18 2014, 1:08 AM
danalbert retitled this revision from to [libcxx] Refactor lit.cfg..
danalbert updated this object.
danalbert edited the test plan for this revision. (Show Details)
danalbert added reviewers: mclow.lists, EricWF.
danalbert added a subscriber: Unknown Object (MLST).

Once again, Phabricator ruined a perfectly good diff. Git shows a nice clean diff, so it's probably easier to apply this patch to your local tree (against r215882, there were a few things submitted while I was working on this that I need to rebase on to) and read it there.

jroelofs added inline comments.
test/lit.cfg
237

This pattern:

self.src_root = self.lit_config.params.get('libcxx_src_root', None)
if self.src_root is None:
    self.src_root = getattr(self.config, 'libcxx_src_root', None)
    if self.src_root is None:
        self.src_root = ...

happens a lot. Maybe it's worth abstracting it out?

emaste added a subscriber: emaste.Aug 18 2014, 11:48 AM
danalbert updated this revision to Diff 12638.Aug 18 2014, 6:35 PM
  • Factor out some of the configuration logic.
EricWF accepted this revision.Aug 20 2014, 6:08 PM
EricWF edited edge metadata.

It makes me very happy to approve this! Now commit it before we have merge conflicts!

A few nits you can choose to ignore or implement:

  • I had trouble finding the start to the configuration class. Maybe add better visual separation.
  • Perhaps we can add better visual separation between methods as well.
  • We lose a lot of space to indentation. I would prefer 2 space tabs. (even though all my editors are set to 4).

Either way LGTM.

This revision is now accepted and ready to land.Aug 20 2014, 6:08 PM
danalbert updated this revision to Diff 12784.Aug 21 2014, 10:10 AM
danalbert edited edge metadata.

Rebased.

danalbert edited the test plan for this revision. (Show Details)Aug 21 2014, 10:35 AM
danalbert closed this revision.Aug 21 2014, 10:37 AM