search

dsharlet / array

C++ multidimensional arrays in the spirit of the STL
Apache License 2.0
198 stars 15 forks source link

Move headers to include/array/* #80

Closed fbleibel-g closed 1 year ago

jiawen commented 1 year ago

Don't forget to update BUILD.bazel.

fbleibel-g commented 1 year ago

Updated. It would be useful to have a travis test for this path too. I could partially test the new include paths, but this doesn't seem to build without the 'clang' toolchain (on my machine, defaults to GCC). It doesn't seem to be as simple as passing CC=clang, there might be other configuration required...

fbleibel-g commented 1 year ago

I'll raise my usual objection to this before approving it: it seems a lot easier for a repo integration script/mechanism to put the files in a subdirectory if it wants to, but it's a lot harder for such a script/mechanism to do the opposite (imagine if we had README.md in both the root of the repo and include/array, I wouldn't want to do this, but an update script would have to consider the possibility). Or if using git submodules/subtrees, you can just make a folder where you want and clone into it, but pulling nested subdirs out is harder and comes with the same risk I mentioned.

One advantage of putting the files in include/array/... is that this defines a standard prefix (#include <array/...>) for headers. My original intent was for this to be on Compiler Explorer, so once (when) this goes live, it would be "the" include path for nda::array files.

Also, yes, the build/install system there is custom and it's not completely obvious how to run a shell script - I could probably figure out how to make the copy, but being able to include array with an additional single include path is going to make things easier for most build systems (that's a change I'd make on the Google side). And you wouldn't want that include path to be littered with not-intended-for-inclusion files...

I think this makes enough things easier that it's more beneficial to make this change than not, but we could revisit for sure.

fbleibel-g commented 1 year ago

Going to "Squash and merge" if there are no objections.

jiawen commented 1 year ago

Going to "Squash and merge" if there are no objections.

Make sure the Bazel tests still pass first? I haven't added them to Travis.

fbleibel-g commented 1 year ago

Updated the header paths in absl/*. 'bazel test //absl:absl_test' passes, and Travis is happy too.