8000 Remove deprecated `std::allocator` members by smolkaj · Pull Request #4364 · p4lang/p4c · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Remove deprecated std::allocator members #4364

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Remove deprecated std::allocator members #4364

wants to merge 1 commit into from

Conversation

smolkaj
Copy link
Member
@smolkaj smolkaj commented Jan 26, 2024 •
edited
Loading

The change replaces std::allocator members that were deprecated in C++17 and removed in C++20, see https://en.cppreference.com/w/cpp/memory/allocator for a detailed list.

@smolkaj smolkaj requested a review from fruffy January 26, 2024 02:11
@smolkaj
Copy link
Member Author
smolkaj commented Jan 26, 2024

Clang format is complaining. Stupid question, how can I run it? I'm using Codespaces.

@smolkaj
Copy link
Member Author
smolkaj commented Jan 26, 2024

I tried make clang-format like in the CI script, but I suppose I need to generate the Makefile first? What's the quickest way to get there?

@vlstill
Copy link
Contributor
vlstill commented Jan 26, 2024

I tried make clang-format like in the CI script, but I suppose I need to generate the Makefile first? What's the quickest way to get there?

Just follow the README: https://github.com/p4lang/p4c#installing-p4c-from-source

@vlstill
Copy link
Contributor
vlstill commented Jan 26, 2024

Btw if you are trying to get C++20 to work, there is much more to do... I've started few weeks ago in #4297 and hit multiple problems. Sadly I don't have much time for that right now.

@fruffy
Copy link
Collaborator
fruffy commented Jan 26, 2024 •
edited
Loading

Clang format is complaining. Stupid question, how can I run it? I'm using Codespaces.

I am not sure how codespaces works, but make clang-format-fix-errors only works with CMake currently. You also need to use pip3 install clang-format==15.0.6

Btw if you are trying to get C++20 to work, there is much more to do... I've started few weeks ago in #4297 and hit multiple problems. Sadly I don't have much time for that right now.

Well, it can't hurt to start in small increments.

@vlstill
Copy link
Contributor
vlstill commented Jan 26, 2024

Well, it can't hurt to start in small increments.

For sure it does. The point of the comment was making sure we don't both end up independently doing very similar changes (this one is quite simple, some of the other may not be...). I will try to separate what can be used from my original PR so it can be merged.

@fruffy
Copy link
Collaborator
fruffy commented Jan 26, 2024

I just tried Codespaces (really neat stuff) and to get it to work you need to run pip3 install --upgrade clang-format==15.0.6 and force reinstallation. The initial codespace environment doesn't ship the binary for some reason.

8000

@smolkaj
Copy link
Member Author
smolkaj commented Jan 27, 2024 •
edited
Loading

Yes, Codespaces is great!

Did you find out a shortcut to fixing the formatting issues, or do I really have to install all p4c dependencies just to format some files? I know nothing about cmake, so would be very grateful about a concrete list of commands I can start from a vanilla Codespaces instance. (Will share the list if I figure it out.)

@fruffy
Copy link
Collaborator
fruffy commented Jan 27, 2024 •
edited
Loading

Yes, Codespaces is great!

Did you find out a shortcut to fixing the formatting issues, or do I really have to install all p4c dependencies just to format some files? I no nothing about cmake, so would be very grateful about a concrete list of commands I can start from a vanilla Codespaces instance. (Will share the list if I figure it out.)

sudo apt update
sudo apt-get install cmake g++ git automake libtool libgc-dev bison flex \
libfl-dev libboost-dev libboost-iostreams-dev \
libboost-graph-dev llvm pkg-config python3 python3-pip \
tcpdump

pip3 install --user -r requirements.txt
mkdir build
cd build
cmake ..
pip3 install --upgrade clang-format==15.0.6
make clang-format-fix-errors

should do the trick. We should really provide some simple install scripts. Everything we have now is built for CI.

I really have to install all p4c dependencies just to format some files?

Unfortunately, some checks are required. Maybe we can refactor CMake in a way to run the linters without requiring installing some dependencies. But that is tricky.

@vlstill
Copy link
Contributor
vlstill commented Jan 29, 2024

Unfortunately, some checks are required. Maybe we can refactor CMake in a way to run the linters without requiring installing some dependencies. But that is tricky.

I would be quite weary of that. If one needs to format .cpp/.h files, it means they have modified them and therefore they should also check it compiles. What might be a good idea is to improve the linter CI output to actually indicate what is wrong/how it should be formatted, so that for very simple cases it would actually make sense to just look at the output from CI and format accordingly, without re-running clang-format locally (I personally tend to run the compiler, but not format, but that is probably my mistake :-D).

@fruffy
Copy link
Collaborator
fruffy commented Jan 29, 2024

I would be quite weary of that. If one needs to format .cpp/.h files, it means they have modified them and therefore they should also check it compiles.

Well, technically CI takes care of that for you. Sometimes you just want to make a quick change. Even in CI we currently need to install all the dependencies just to run a linter check. That doesn't seem right.

What might be a good idea is to improve the linter CI output to actually indicate what is wrong/how it should be formatted

We do have this, clang-format is just a bit hard to read sometimes. This is the current output:

Formatting [706/855] /home/runner/work/p4c/p4c/lib/ordered_map.h
/home/runner/work/p4c/p4c/lib/ordered_map.h:69:54: error: code should be clang-formatted [-Wclang-format-violations]
    using map_alloc = typename std::allocator_traits<
                                                     ^
/home/runner/work/p4c/p4c/lib/ordered_map.h:70:39: error: code should be clang-formatted [-Wclang-format-violations]
        ALLOC>::template rebind_alloc<std::pair<const K *const, list_iterator>>;
                                      ^
Formatting [707/855] /home/runner/work/p4c/p4c/lib/ordered_set.h
/home/runner/work/p4c/p4c/lib/ordered_set.h:60:54: error: code should be clang-formatted [-Wclang-format-violations]
    using map_alloc = typename std::allocator_traits<
                                                     ^
/home/runner/work/p4c/p4c/lib/ordered_set.h:61:39: error: code should be clang-formatted [-Wclang-format-violations]
        ALLOC>::template rebind_alloc<std::pair<const T *const, list_iterator>>;

Although I am sure we can make this prettier.

@vlstill vlstill mentioned this pull request Jan 29, 2024
Merged
@smolkaj
Copy link
Member Author
smolkaj commented Jan 29, 2024

Thanks a lot, Fabian! Looks like #4373 is taking care of things -- thanks @vistill -- but this will be very helpful for future reference.

I also didn't realize that clang-format actually showed me how it wants things formatted, as you noted the output is quite subtle.

+1 that making sure that code compiles is the job of CI.

@smolkaj smolkaj closed this Jan 29, 2024
@fruffy fruffy deleted the allocators branch February 10, 2024 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fruffy fruffy fruffy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@smolkaj @vlstill @fruffy
0