ID_* symbols are now constexpr #2133

kroening · 2018-04-28T12:21:54Z

This partially reverts 38782bd.

The idea is that the ID_* symbols can be constexpr, which may help a sophisticated compiler. The question whether it does should be investigated. A disadvantage is that the symbols need to be parsed twice, which may affect compile time.

tautschnig · 2018-04-29T18:09:23Z

My understanding of a past discussion of introducing constexpr on these was that we could do switch(id) { case ID_...: case ID_... } - is that genuinely the case with this patch applied? Else the value doesn't seem particularly obvious to me.

kroening · 2018-04-30T09:29:15Z

Yes, you could, if you also enable the #if 0 in line 41 in src/util/dstring.h.

The key disadvantage is that this allows also all other conversions to int, e.g., in an if, or a plain assignment. The other problem is that this will never work with std::string, i.e., once we start using switch, then USE_DSTRING must be on from thereon.

It is not clear to me what the benefit of enabling the compiler to have more compile-time constants is in general. This may also change over time.

smowton · 2018-04-30T11:42:15Z

src/util/irep_ids.h

+#include "irep_ids.def" // NOLINT(build/include)
+};
+
+#ifdef __GNUC__


Clang, MSVC 2015+ also have this. Suggest stealing the testing logic used at https://github.com/diffblue/cbmc/blob/develop/src/nonstd/optional.hpp#L87

kroening · 2018-05-25T10:25:59Z

I've just added another commit that allows conversion to the enum class. This prevents accidental conversion to int, pointers, bool etc., but enables switch(some_id) { case ID_...:; }. This is a serious benefit in my view.

The price is that we have to ditch versions of Visual C++ prior to 2015. Also, we cannot use std::string any longer as alternative to the hashed strings.

Views?

smowton · 2018-05-29T07:27:25Z

Do you have any feel on how many Windows users are out there, and if so what their limitations are? We currently require MSVC2013, which is an odd version, having sort-of-but-broken C++11 support, so it would be a surprising place for people to be stuck

kroening · 2018-05-29T08:01:09Z

There are some left, but are getting fewer and fewer. I'll send emails.

kroening · 2018-05-29T08:02:44Z

My current view is to merge this, but not yet use switch(...) in cbmc itself for a release or two. Dependent software that is gcc/clang-only can then start using this now.

tautschnig · 2018-05-29T11:42:21Z

So what is the "enhancement" for now? Smaller binaries, faster code, something else? For sure theory tells us that the switch/case should be faster than a chain of if/else, but is this also true in practice? With dstringt being the most fundamental data structure, changes here should be accompanied by data.

allredj

This PR failed Diffblue compatibility checks (cbmc commit: b21d45b).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/85621224
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.

Common spurious failures:

the cbmc commit has disappeared in the mean time (e.g. in a force-push)
the author is not in the list of contributors (e.g. first-time contributors).

allredj

This PR failed Diffblue compatibility checks (cbmc commit: 8765b34).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/86396183
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.

Common spurious failures:

the cbmc commit has disappeared in the mean time (e.g. in a force-push)
the author is not in the list of contributors (e.g. first-time contributors).

allredj

This PR failed Diffblue compatibility checks (cbmc commit: 651af68).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/87621638
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.

Common spurious failures:

the cbmc commit has disappeared in the mean time (e.g. in a force-push)
the author is not in the list of contributors (e.g. first-time contributors).

The incompatibility may have been introduced by an earlier PR. In that case merging this
PR should be avoided unless it fixes the current incompatibility.

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: 00eb39b).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/94117295

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: a9af8bb).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/97924137

Upgrading to VS2015 enables #3862, noexcept (e.g. #2502), UTF8 strings and constexpr (#2133).

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: d358fee).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/98233189

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: 3ce1575).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/99567549

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: c3ba809).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/100693494

TGWDB · 2023-05-03T15:20:57Z

Closing due to age (no further comment on PR content), please reopen with rebase on develop if you intent to continue this work.

kroening added the do not merge label Apr 28, 2018

kroening requested a review from tautschnig April 28, 2018 12:21

kroening requested review from peterschrammel and smowton as code owners April 28, 2018 12:21

kroening force-pushed the ID_constexpr branch from 79dbe42 to dc5f031 Compare April 28, 2018 12:24

smowton reviewed Apr 30, 2018

View reviewed changes

tautschnig assigned kroening May 5, 2018

kroening force-pushed the ID_constexpr branch from dc5f031 to 0e113ef Compare May 25, 2018 10:24

kroening assigned tautschnig, peterschrammel and smowton and unassigned kroening, tautschnig and peterschrammel May 25, 2018

kroening added enhancement question and removed do not merge labels May 26, 2018

tautschnig assigned kroening and unassigned smowton, tautschnig and peterschrammel Jun 1, 2018

kroening force-pushed the ID_constexpr branch from 4005351 to 790bf49 Compare August 24, 2018 09:20

kroening force-pushed the ID_constexpr branch from 790bf49 to b21d45b Compare September 21, 2018 21:30

allredj reviewed Sep 22, 2018

View reviewed changes

kroening force-pushed the ID_constexpr branch from b21d45b to 8765b34 Compare September 30, 2018 14:56

allredj reviewed Sep 30, 2018

View reviewed changes

kroening force-pushed the ID_constexpr branch 2 times, most recently from 3376932 to 651af68 Compare October 11, 2018 10:16

allredj reviewed Oct 11, 2018

View reviewed changes

kroening force-pushed the ID_constexpr branch 2 times, most recently from 4816beb to 00eb39b Compare December 9, 2018 14:03

allredj reviewed Dec 9, 2018

View reviewed changes

kroening force-pushed the ID_constexpr branch from 00eb39b to a9af8bb Compare January 19, 2019 13:05

allredj reviewed Jan 19, 2019

View reviewed changes

kroening pushed a commit that referenced this pull request Jan 22, 2019

Drop the desire to do VS2013 builds

1547ab0

Upgrading to VS2015 enables #3862, noexcept (e.g. #2502), UTF8 strings and constexpr (#2133).

kroening mentioned this pull request Jan 22, 2019

Drop the desire to do VS2013 builds #3894

Merged

2 tasks

kroening force-pushed the ID_constexpr branch from a9af8bb to d358fee Compare January 22, 2019 20:12

allredj reviewed Jan 22, 2019

View reviewed changes

kroening force-pushed the ID_constexpr branch from d358fee to 3ce1575 Compare February 3, 2019 19:52

allredj reviewed Feb 3, 2019

View reviewed changes

kroening force-pushed the ID_constexpr branch from 3ce1575 to c3ba809 Compare February 12, 2019 22:01

allredj reviewed Feb 13, 2019

View reviewed changes

kroening force-pushed the ID_constexpr branch from c3ba809 to 597330e Compare February 23, 2019 16:11

kroening force-pushed the ID_constexpr branch 2 times, most recently from 65e7981 to 57751ae Compare March 17, 2019 14:06

kroening force-pushed the ID_constexpr branch from 57751ae to 75ac3db Compare May 1, 2019 15:12

Daniel Kroening added 2 commits July 30, 2019 12:00

ID_* symbols are now constexpr

a4fdcd8

enable conversion from irep_idt to enum class

73a20f4

kroening force-pushed the ID_constexpr branch from 75ac3db to 73a20f4 Compare July 30, 2019 11:00

TGWDB closed this May 3, 2023

ID_* symbols are now constexpr #2133

ID_* symbols are now constexpr #2133

Uh oh!

Conversation

kroening commented Apr 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tautschnig commented Apr 29, 2018

Uh oh!

kroening commented Apr 30, 2018

Uh oh!

smowton Apr 30, 2018

Choose a reason for hiding this comment

Uh oh!

kroening commented May 25, 2018

Uh oh!

smowton commented May 29, 2018

Uh oh!

kroening commented May 29, 2018

Uh oh!

kroening commented May 29, 2018

Uh oh!

tautschnig commented May 29, 2018

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

TGWDB commented May 3, 2023

Uh oh!

Uh oh!

kroening commented Apr 28, 2018 •

edited

Loading