search

daphne-eu / daphne

DAPHNE: An Open and Extensible System Infrastructure for Integrated Data Analysis Pipelines
Apache License 2.0
67 stars 62 forks source link

830 compilation with clang #831

Open corepointer opened 1 month ago

corepointer commented 1 month ago

The commits contained in this PR fix compilation with Clang (tested version 18) and also the LLVM runtime issues with anonymous name spaces.

I tried to form the commits per problem, although they are related to the same issue, so squashing them might be inappropriate imho :thinking:

ToDo:

philipportner commented 1 month ago

I think these are unwanted whitespace changes in RewriteToCallKernelOpPass.cpp? I'm not sure which editor you use but the .clang-format file should handle this. 1193 changes compared to 7 when hiding whitespace.

philipportner commented 1 month ago

IMO the changes should be squashed into a single commit. I think the C++20 commit should be a separate commit/PR outside of this PR and merged before if necessary for clang.

corepointer commented 1 month ago

I think these are unwanted whitespace changes in RewriteToCallKernelOpPass.cpp? I'm not sure which editor you use but the .clang-format file should handle this. 1193 changes compared to 7 when hiding whitespace.

I noticed the file with the big formatting changes and thought that was due to clang-format kicking in. But I didn't check tbh. I'm using CLion.

corepointer commented 1 month ago

IMO the changes should be squashed into a single commit. I think the C++20 commit should be a separate commit/PR outside of this PR and merged before if necessary for clang.

The commits are on the same topic but tackle distinct problems. As I read frequently in reviews to separate this and that into separate commits I thought this is the right thing to do here.

philipportner commented 1 month ago

I noticed the file with the big formatting changes and thought that was due to clang-format kicking in. But I didn't check tbh. I'm using CLion.

Maybe it's not been automatically enabled by CLion?

corepointer commented 1 month ago

The C++20 change is in a separate commit for the same reason. Can be cherry picked to main before handling the PR if you want ;-)

corepointer commented 1 month ago

I noticed the file with the big formatting changes and thought that was due to clang-format kicking in. But I didn't check tbh. I'm using CLion.

Maybe it's not been automatically enabled by CLion?

That's what I thought would happen if the .clang-format file is there :thinking:

philipportner commented 1 month ago

FYI: I've formatted the code with clang-format version 18.1.8 (++20240731025011+3b5b5c1ec4a3-1~exp1~20240731145104.143).

philipportner commented 1 month ago

Thanks for this patch @corepointer .

Few questions:

  1. Which version of clang did you use to test this?
  2. Did you get any warnings?
  3. Should I be able to switch between specifying --clang and omitting it without doing anything?
  4. If no to 3, what would I need to do to switch between clang and gcc? I would assume a rm -rf build should be enough?
corepointer commented 1 month ago

Thanks for this patch @corepointer .

:+1:

Few questions:

1. Which version of `clang` did you use to test this?

Version 18.1.3 - mentioned in the commit message :point_up: 

2. Did you get any warnings?

Tons of warnings. Besides numerous from our code(which I started to fix here and there in minor commits), there are quite many originating from our LLVM/MLIR snapshot and from the JSON library we use.

3. Should I be able to switch between specifying `--clang` and omitting it without doing anything?

4. If no to 3, what would I need to do to switch between `clang` and `gcc`? I would assume a `rm -rf build` should be enough?

No, you need to remove the current build files (either rm -rf build or ./bulid.sh --clean -y)