Refactor Util::Enumerator #4513

asl · 2024-03-07T23:47:00Z

Simplify interface
Introduce special-case for ICastable for AsEnumerator
Remove non-sense std::function type-erasure for FilterEnumerator, make it less memory hog
Make ConcatEnumerator sane, do not do tons of memory allocations each time ConcatEnumerator is created
Some convenient helpers (enumerate / concat)
Make EnumeratorHandle behave as ForwardIterator, so STL insert, etc. are working with Enumerator
Make sure Values and other things from lib/map.h could be used with Enumerator, improve here const-correctness & remove deprecated std::iterator usage in favor of iterator traits
Normalize doxygen comments and reorganize code inside lib/enumerator.h, make it cstring-less

asl · 2024-03-07T23:47:45Z

@fruffy This is the change that gives that p4tools testcase speedup due to removal of dynamic_cast.

asl · 2024-03-07T23:51:35Z

@fruffy What is clang-format version on CI? It fails checks, however, code is clearly clang-format clean for me (with clang-format 17.0.6)

asl · 2024-03-07T23:56:23Z

Compilation failed due to gcc bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90031

fruffy · 2024-03-08T02:15:56Z

@fruffy What is clang-format version on CI? It fails checks, however, code is clearly clang-format clean for me (with clang-format 17.0.6)

We pin it to 15.0.6 https://github.com/p4lang/p4c/blob/main/requirements.txt#L7
But there is no reason not to upgrade it. Just needs to be consistent so people do not get confused.

fruffy

Thanks! I do not have many comments on the technical side, it's probably best other stakeholders look at the changes here.

fruffy · 2024-03-08T19:19:36Z

lib/enumerator.h

        : Enumerator<typename Iter::value_type>(),
          begin(begin),
          end(end),
          current(begin),
          name(name) {}

- public:
-    cstring toString() const { return this->name + ":" + this->stateName(); }
+    [[nodiscard]] std::string toString() const {


Hmm most other toString functions return a cstring. I am not certain whether we should refactor this spotwise or do it all at once it one single PR.

Well, I'd go for small iterative changes. Otherwise this (cstring change) would end with something of enormous size that is impossible to review / digest

Fine with me. We should define some form of heuristic to decide when to use a cstring. toString might be fairly benign for example.

Yes. I'm planning to do first sweep one day to get rid of most obvious cases (e.g. we do not need to cstring of format strings or jsons). Then we can discuss the proper API (cstring vs std::string vs std::string_view usage)

Then we can discuss the proper API (cstring vs std::string vs std::string_view usage)

And document it somewhere 🙂

grg

Looks okay to me too.

frontends/common/resolveReferences/resolveReferences.cpp

lib/enumerator.h

grg · 2024-03-08T19:54:58Z

lib/enumerator.h

        : Enumerator<typename Iter::value_type>(),
          begin(begin),
          end(end),
          current(begin),
          name(name) {}

- public:
-    cstring toString() const { return this->name + ":" + this->stateName(); }
+    [[nodiscard]] std::string toString() const {


Then we can discuss the proper API (cstring vs std::string vs std::string_view usage)

And document it somewhere 🙂

asl · 2024-03-12T16:18:50Z

@vlstill @fruffy Will you please take a look when you have a chance? Thanks!

vlstill

Looks good overall, but I have concerns about some details.

ir/ir.cpp

lib/enumerator.h

lib/map.h

tools/ir-generator/irclass.cpp

ChrisDodd

Nice cleanups!

Though I think ideally we'd like to move away from these java enumerators to more static C++-style ones.

asl · 2024-03-14T19:50:12Z

Nice cleanups!

Though I think ideally we'd like to move away from these java enumerators to more static C++-style ones.

This is the plan. But one step at a time. We rely on this type-erased things heavily in few places...

asl · 2024-03-15T19:13:32Z

@vlstill Refactored MapEnumerator as well. Please take a look when you will have a chance. CTAD + std::invoke_result made the task simple

…used as a bit non-standard InputIterator

…ed to std::function indirection and type erasure

…behavior

Ensure Values() could be used like "container".

…ile there

Co-authored-by: Glen Gibb <glen.gibb@alumni.stanford.edu>

asl · 2024-03-22T06:22:08Z

@vlstill Will you please check the refined PR. Does it address your comments? Thanks!

vlstill

Thank you for the changes! Sorry I forgot about needing to re-review this one.

lib/map.h

Restore parentheses that were accidentally dropped from begin call as part of recent code improvements (#4513).

* restore missing parentheses Restore parentheses that were accidentally dropped from begin call as part of recent code improvements (#4513). * add basic test for ValuesForKeys from map.h

asl requested review from fruffy and vlstill March 7, 2024 23:47

asl force-pushed the enum-trash-refactor branch from 0a1b850 to 0c639af Compare March 7, 2024 23:47

asl mentioned this pull request Mar 8, 2024

Get rid of dynamic_cast in filtering enumerator #4478

Closed

fruffy requested review from grg and ChrisDodd March 8, 2024 19:22

fruffy reviewed Mar 8, 2024

View reviewed changes

grg reviewed Mar 8, 2024

View reviewed changes

asl force-pushed the enum-trash-refactor branch from dec1c18 to 3707662 Compare March 9, 2024 01:08

grg approved these changes Mar 9, 2024

View reviewed changes

fruffy approved these changes Mar 12, 2024

View reviewed changes

vlstill requested changes Mar 12, 2024

View reviewed changes

asl force-pushed the enum-trash-refactor branch 2 times, most recently from f3c91ac to fc45577 Compare March 12, 2024 20:45

asl requested a review from vlstill March 12, 2024 20:46

ChrisDodd approved these changes Mar 14, 2024

View reviewed changes

fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Mar 15, 2024

asl force-pushed the enum-trash-refactor branch from 60058ef to 12a8cc7 Compare March 15, 2024 19:12

asl force-pushed the enum-trash-refactor branch from 12a8cc7 to 563d685 Compare March 19, 2024 21:13

asl added 4 commits March 21, 2024 12:08

Simplify

a9799f7

Ensure AsEnumerator could use lightweight RTTI when available

53837b2

Make EnumeratorHandle adopt iterator concept better. Now it could be …

433b1bb

…used as a bit non-standard InputIterator

Allow FilterEnumerator to own predicate. This removes overheads relat…

b0f4e9e

…ed to std::function indirection and type erasure

asl and others added 15 commits March 21, 2024 12:08

Rename GenericEnumerator => IteratorEnumerator to better outline the …

d26cec1

…behavior

Add type-deducing enumerate() helpers

7ae889c

Get rid of deprecated std::iterator usage. Improve const-correctness.

a4c295f

Ensure Values() could be used like "container".

Use enumerate() instead of chatty createEnumerator(). Some cleanup wh…

9b6b246

…ile there

Deprecate createEnumerator()

7d7ab09

No need to use cstring in enumerator and pay overheads

337b0ca

Refactor ConcatEnumerator not to be memory hog

8bf0c50

Simplify

6912d9e

Get rid of static emptyVector

2e38454

Normalize comments and simplify code organization

a9aa1f2

Try to workaround gcc bug

15bb8b6

Fix typo

e9fd69d

Co-authored-by: Glen Gibb <glen.gibb@alumni.stanford.edu>

Address review comments

87ef087

Refactored MapEnumerator to take mapper by value

2cd001a

Be correct wrt const iterators in map Values()

eb2cdab

asl force-pushed the enum-trash-refactor branch 2 times, most recently from 53d1ef0 to eb2cdab Compare March 21, 2024 19:17

Pacify lint

6cc0eab

vlstill approved these changes Mar 22, 2024

View reviewed changes

lib/map.h Outdated Show resolved Hide resolved

Add missed cases

2cf6c1b

asl enabled auto-merge March 22, 2024 10:24

asl added this pull request to the merge queue Mar 22, 2024

Merged via the queue into p4lang:main with commit 66d7e6e Mar 22, 2024
17 checks passed

asl deleted the enum-trash-refactor branch March 22, 2024 12:22

grg added a commit that referenced this pull request Apr 5, 2024

restore missing parentheses

09782c3

Restore parentheses that were accidentally dropped from begin call as part of recent code improvements (#4513).

grg mentioned this pull request Apr 5, 2024

restore missing parentheses #4600

Merged

asl linked an issue Apr 18, 2024 that may be closed by this pull request

Get rid of dynamic_cast in filtering enumerator #4478

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Util::Enumerator #4513

Refactor Util::Enumerator #4513

asl commented Mar 7, 2024 •

edited

Loading

asl commented Mar 7, 2024

asl commented Mar 7, 2024

asl commented Mar 7, 2024

fruffy commented Mar 8, 2024

fruffy left a comment

fruffy Mar 8, 2024

asl Mar 8, 2024

fruffy Mar 8, 2024

asl Mar 8, 2024

grg Mar 8, 2024

grg left a comment

grg Mar 8, 2024

asl commented Mar 12, 2024

vlstill left a comment

ChrisDodd left a comment

asl commented Mar 14, 2024

asl commented Mar 15, 2024

asl commented Mar 22, 2024

vlstill left a comment

Refactor Util::Enumerator #4513

Refactor Util::Enumerator #4513

Conversation

asl commented Mar 7, 2024 • edited Loading

asl commented Mar 7, 2024

asl commented Mar 7, 2024

asl commented Mar 7, 2024

fruffy commented Mar 8, 2024

fruffy left a comment

Choose a reason for hiding this comment

fruffy Mar 8, 2024

Choose a reason for hiding this comment

asl Mar 8, 2024

Choose a reason for hiding this comment

fruffy Mar 8, 2024

Choose a reason for hiding this comment

asl Mar 8, 2024

Choose a reason for hiding this comment

grg Mar 8, 2024

Choose a reason for hiding this comment

grg left a comment

Choose a reason for hiding this comment

grg Mar 8, 2024

Choose a reason for hiding this comment

asl commented Mar 12, 2024

vlstill left a comment

Choose a reason for hiding this comment

ChrisDodd left a comment

Choose a reason for hiding this comment

asl commented Mar 14, 2024

asl commented Mar 15, 2024

asl commented Mar 22, 2024

vlstill left a comment

Choose a reason for hiding this comment

asl commented Mar 7, 2024 •

edited

Loading