[libcxx-commits] [PATCH] D104492: [libc++][pstl] Implement tag dispatching

[Ruslan Arutyunyan via Phabricator via libcxx-commits]

rarutyun added inline comments.


================
Comment at: pstl/include/pstl/internal/algorithm_fwd.h:19
 
 _PSTL_HIDE_FROM_ABI_PUSH
 
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > Quuxplusone wrote:
> > > I believe you need to add `pstl/include` to
> > > https://github.com/google/llvm-premerge-checks/blob/main/scripts/clang-tidy.ignore
> > > (Although why this file is in a Google repo instead of the llvm-project repo, I don't know!)
> > Additionally, it looks like these files may be nested one level too deep. I think this should be `pstl/include/algorithm_fwd.h` not `pstl/include/pstl/include/algorithm_fwd.h`.
> @zoecarver: No, I bet the nesting is right. Notice that it's
> ```
>     pstl/include/pstl/internal/algorithm_fwd.h
> not                   ^^^^^^^^
>     pstl/include/pstl/include/algorithm_fwd.h
> ```
> The pattern of "foo/include/foo/" ends up being pretty common in codebases I've seen, because there'll be a "foo/src" and a "foo/include", and then in order to make `#include <foo/bar.h>` to work after `-I foo/include`, you need to put your actual header file in "foo/include/foo/bar.h".
>I believe you need to add pstl/include to
>https://github.com/google/llvm-premerge-checks/blob/main/scripts/clang-tidy.ignore
>(Although why this file is in a Google repo instead of the llvm-project repo, I don't know!)

I believe we will do that (if necessary) when enable CI for Parallel STL (which is one of the next steps after merging this patch). Currently I propose to just merge this changes ignoring clang-tidy (as it was done for previous patches as well)


================
Comment at: pstl/include/pstl/internal/algorithm_fwd.h:19
 
 _PSTL_HIDE_FROM_ABI_PUSH
 
----------------
rarutyun wrote:
> Quuxplusone wrote:
> > zoecarver wrote:
> > > Quuxplusone wrote:
> > > > I believe you need to add `pstl/include` to
> > > > https://github.com/google/llvm-premerge-checks/blob/main/scripts/clang-tidy.ignore
> > > > (Although why this file is in a Google repo instead of the llvm-project repo, I don't know!)
> > > Additionally, it looks like these files may be nested one level too deep. I think this should be `pstl/include/algorithm_fwd.h` not `pstl/include/pstl/include/algorithm_fwd.h`.
> > @zoecarver: No, I bet the nesting is right. Notice that it's
> > ```
> >     pstl/include/pstl/internal/algorithm_fwd.h
> > not                   ^^^^^^^^
> >     pstl/include/pstl/include/algorithm_fwd.h
> > ```
> > The pattern of "foo/include/foo/" ends up being pretty common in codebases I've seen, because there'll be a "foo/src" and a "foo/include", and then in order to make `#include <foo/bar.h>` to work after `-I foo/include`, you need to put your actual header file in "foo/include/foo/bar.h".
> >I believe you need to add pstl/include to
> >https://github.com/google/llvm-premerge-checks/blob/main/scripts/clang-tidy.ignore
> >(Although why this file is in a Google repo instead of the llvm-project repo, I don't know!)
> 
> I believe we will do that (if necessary) when enable CI for Parallel STL (which is one of the next steps after merging this patch). Currently I propose to just merge this changes ignoring clang-tidy (as it was done for previous patches as well)
```    pstl/include/pstl/internal/algorithm_fwd.h
not                   ^^^^^^^^
    pstl/include/pstl/include/algorithm_fwd.h
```
I agree with @Quuxplusone  here


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104492/new/

https://reviews.llvm.org/D104492