This is an archive of the discontinued LLVM Phabricator instance.

Changes from integrating PSTL with libstdc++
AbandonedPublic

Authored by rodgert on Mar 1 2019, 4:03 PM.

Details

Reviewers
ldionne
Summary

This is a combination of the following:

  • Remove unused include - pstl_config.h is not required
  • Introduce forward declarations - necessary to break circular includes when introduced to the standard library's, e.g. - <algorithm> <numeric> <memory>
  • Deprecate non-cmake build and test suite Makefile based build is not a LLVM standard Original test suite does not conform to libc++'s standards Scripts for integrating tests into libstdc++ assume libc++'s test layout

Event Timeline

rodgert created this revision.Mar 1 2019, 4:03 PM

Do you think you could split this change into smaller changes? Perhaps one that removes the support for the Makefile tests, one that introduces forward declarations, and one that renames the test suite files?

Do you think you could split this change into smaller changes? Perhaps one that removes the support for the Makefile tests, one that introduces forward declarations, and one that renames the test suite files?

I tried to git rebase --interactive my way into something along those lines, but the series of commits which were squashed into this one made the process fairly messy. I can certainly split this one into two changes though, one that introduces the forward declarations (it also removes some unused includes), and one that simultaneously deprecates the support for the Makefile tests and reintroduces them with the new structure. Would that work for you?

I tried to git rebase --interactive my way into something along those lines, but the series of commits which were squashed into this one made the process fairly messy. I can certainly split this one into two changes though, one that introduces the forward declarations (it also removes some unused includes), and one that simultaneously deprecates the support for the Makefile tests and reintroduces them with the new structure. Would that work for you?

Yes.

MikeDvorskiy added inline comments.
include/pstl/internal/algorithm_fwd.h
2

Thomas, what's a reason to rename the files with forward declarations? (glue_algorithm_defs.h, glue_numeric_defs.h, glue_memory_defs.h to algorithm_fwd.h, numeric_fwd.h, memory_fwd.h)

pstl_config.h is not required

Thomas, could you please clarify it?

The fact is the file contains significant macros, which are used in simd bricks, and the other useful macros.

rodgert marked an inline comment as done.Mar 5 2019, 6:43 AM

pstl_config.h is not required

Thomas, could you please clarify it?

The fact is the file contains significant macros, which are used in simd bricks, and the other useful macros.

The fact is, it is already included by another file here, so this include is redundant. Removing the include here is the correct thing to do, also because this config file's contents become part of the standard library's configuration when included in libstdc++ (and almost certainly the same with libc++).

include/pstl/internal/algorithm_fwd.h
2

Look carefully at the files, they are *NOT* the same thing.

rodgert abandoned this revision.Mar 5 2019, 1:52 PM

Will resubmit (as discussed) as a couple of smaller patches

build/mingw.inc