Details
- Reviewers
ldionne Mordante - Group Reviewers
Restricted Project - Commits
- rG5c6be1d48c15: [libc++][PSTL] Add design docs
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Would be nice to include
(a) the unabbreviated form of PSTL
and
(b) a link to the project
I had to google it to figure out what this note was about :-)
Thanks for writing the documentation! Some remarks.
+1
| libcxx/docs/DesignDocs/PSTL.rst | ||
|---|---|---|
| 5 ↗ | (On Diff #514049) | |
| 9–10 ↗ | (On Diff #514049) | In a design documentation I expect some information behind the design choices. Can you add that information. |
| 20 ↗ | (On Diff #514049) | Maybe the document should contain some information on how we intend to keep in sync with upstream. |
This is a bit ambiguous. Do you mean "Parallel STL"? If yes, I'm not sure it results in less confusion, since it's mostly referred to as the PSTL. The subdirectory is also called pstl.
(b) a link to the project
Where exactly would you like to have a link to? AFAIK there is no website for the LLVM PSTL, and linking to the Intel PSTL would be a bit confusing IMO.
I had to google it to figure out what this note was about :-)
| libcxx/docs/DesignDocs/PSTL.rst | ||
|---|---|---|
| 9–10 ↗ | (On Diff #514049) | The reason for basically all of the removals is "we don't need the flexibility that the extra complexity provides, but we have to modify the code to fit our needs". Do you have any wording suggestions? |
| 20 ↗ | (On Diff #514049) | We can add that once we know how we will do that. Right now the only plan is to have matching patches for libc++. The PSTL isn't exactly active. |
(a) the unabbreviated form of PSTL
This is a bit ambiguous. Do you mean "Parallel STL"? If yes, I'm not sure it results in less confusion, since it's mostly referred to as the PSTL. The subdirectory is also called pstl.
Just something like PSTL (Parallel STL)
(b) a link to the project
Where exactly would you like to have a link to? AFAIK there is no website for the LLVM PSTL, and linking to the Intel PSTL would be a bit confusing IMO.
I had to google it to figure out what this note was about :-)
Ah, I see, fair enough. (The way the note is worded I thought it was about including and modifying an external project into LLVM)
I think calling it Parallel STL once will help. Somebody not familiar with the name directly has a good idea what this is about.
I don't feel we need to spell out STL, since that is quite a familiar C++ term.
LGTM, modulo one nit. I leave the final approval to Louis.
| libcxx/docs/DesignDocs/PSTLIntegration.rst | ||
|---|---|---|
| 10 | Since you talk about PSTL I really would like a link to the original source. I really like this new rationale, this makes it very clear why you opted to go in this direction. Thanks! | |
| libcxx/docs/DesignDocs/PSTLIntegration.rst | ||
|---|---|---|
| 10 | It's still not clear to me where you would like to have a link to, since we don't have any online documentation about the PSTL. Or do you mean some Intel-specific stuff? I don't know where that lives, and e.g. https://www.intel.com/content/www/us/en/developer/articles/guide/get-started-with-parallel-stl.html makes the claim that C++11 is required, which is simply wrong for our purposes. | |
LGTM. This document is going to be short lived anyway, once the integration is done the details of how we did it is not going to be so relevant.
Since you talk about PSTL I really would like a link to the original source.
I really like this new rationale, this makes it very clear why you opted to go in this direction. Thanks!