Implementation of standard C/C++ fixed width, minimum width, and maximum width types #6052

jsinglet · 2021-06-09T20:18:23Z

Implementation of standard C/C++ fixed width, minimum width, and maximum width types. Also some modification to the way intmax_t and uintmax_t are organized.

…ypes

rdmarsh2

Thanks! Most of this looks good to me. I've requested a few changes to the doc comments, and you'll also need to autoformat your QL files and fix the failing test before we can merge this.

rdmarsh2 · 2021-06-09T20:44:57Z

cpp/ql/src/semmle/code/cpp/commons/CommonType.qll

@@ -63,13 +63,258 @@ class Ptrdiff_t extends Type {
  override string getAPrimaryQlClass() { result = "Ptrdiff_t" }
 }

+/**
+ * A common base type for describing the C/C++ fixed-width numeric types


When possible, qldoc should describe the elements in the target language rather than the QL class.

Suggested change

* A common base type for describing the C/C++ fixed-width numeric types

* A C/C++ fixed-width numeric type, such as `int8_t`

rdmarsh2 · 2021-06-09T20:55:41Z

cpp/ql/src/semmle/code/cpp/commons/CommonType.qll

+}
+
+/**
+ * A common base type for describing the C/C++ minimum-width numeric types.


Suggested change

* A common base type for describing the C/C++ minimum-width numeric types.

* A C/C++ minimum-width numeric type, such as `int_least8_t` or `uint_fast16_t`.

rdmarsh2 · 2021-06-09T20:56:21Z

cpp/ql/src/semmle/code/cpp/commons/CommonType.qll

+}
+
+/**
+ * A common base type for describing the C/C++ maximum-width numeric types.


Suggested change

* A common base type for describing the C/C++ maximum-width numeric types.

* A C/C++ maximum-width numeric type, either `intmax_t` or `uintmax_t`.

rdmarsh2 · 2021-06-09T21:02:32Z

cpp/ql/src/semmle/code/cpp/commons/CommonType.qll

+}
+
+/**
+ * A common base type for describing enum types that are based on fixed-width types.


Suggested change

* A common base type for describing enum types that are based on fixed-width types.

* An enum type based on a fixed-width integer type. For instance, `enum e: uint8_t = { a, b };`

rdmarsh2 · 2021-06-09T21:04:50Z

cpp/ql/test/library-tests/types/types/types.cpp

@@ -80,3 +80,6 @@ size_t st;
 ssize_t sst;
 ptrdiff_t pdt;
 wchar_t wct;
+
+std::int8_t i8;


You'll need to declare std::int8_t here so the test can be extracted properly.

@rdmarsh2 -- ack, this was a mistake; that definition isn't supposed to be there.

jbj · 2021-06-10T13:49:50Z

cpp/ql/src/semmle/code/cpp/commons/CommonType.qll

+/**
+ * A common base type for describing the C/C++ fixed-width numeric types
+ */
+abstract class FixedWidthIntegralType extends UserType {


We've made a big push to avoid public abstract classes in the base libraries, including this file. See https://github.com/github/codeql-c-team/issues/60 for some motivation and context. Abstract classes are great for the library-internal implementation but painful and dangerous to work with for consumers of the library. For code in the base libraries, the balance is toward reducing surprises for the library's consumers.

What approach do you recommend here? This class won't behave correctly in its current form without abstract.

I'd suggest going with a union approach similar to what we've done here: #3940

i.e., make a TFixedWidthIntegralType that's a union of all the classes that currently implement the abstract class, and then make a FixedWidthIntegralType class that extends TFixedWidthIntegralType (and UserType).

@jbj @geoffw0 @MathiasVP -- Please see the latest push to this PR which incorporates a bunch of suggestions from @jbj for resolving this issue. Please let me know so we can come to a consensus.

Also, I'm failing the changelog check but it doesn't look like it's been updated in a while. Shall I create a new directory and note these changes (if so, what directory/version to add?) or omit the changelog entry?

reading @MathiasVP -- I'll update and push again.

We've changed (🥁) the way we do change-notes (which is why that directory hasn't been updated in a while). See an example here on how we do it now (specifically, the file cpp/change-notes/2021-10-05-comparison-with-wider-type.md created by that PR).

Thanks @MathiasVP!

jbj

LGTM modulo a typo fix

jbj · 2021-06-11T12:54:33Z

cpp/ql/src/semmle/code/cpp/commons/CommonType.qll

+class FixedWidthIntegralType extends TFixedWidthIntegralType {
+  FixedWidthIntegralType() { this instanceof TFixedWidthIntegralType }


This worked!? I'm baffled. Did you know this would work, @MathiasVP? It seems like we can derive from an abstract class without extending it by using extends T and this instanceof T in combination (without abstract it would be redundant).

I think this is Pavel's "enshrinkening subtyping" pattern, right? It seems to fit this use-case very well, indeed.

Aha. For the record, this pattern was discussed in https://github.slack.com/archives/CP0LHP150/p1606300178023900 (and I was apparently in the discussion).

The ever-talented Mr. @lcartey showed me this pattern in a coding session where we were examining all of the options for representing this (and also the history of why we avoid it). We compared it against some other approaches, for example, one based around ADTs, but in the end this one seems to strike a nice balance between convenience and maintainability.

cpp/change-notes/2021-06-10-std-types.md

Co-authored-by: Jonas Jensen <jbj@github.com>

jbj · 2021-06-11T14:00:33Z

The tests are running as I'm writing this, but I believe earlier there was an autoformat error from "CPP Language Tests 2 (GitHub Actions)".

Also, the QLDoc Checks are asking you to move (no need to copy) the QLDoc from the private abstract classes to the public ones.

adding standard C/C++ fixed width, minimum width, and maximum width t…

28e2cdb

…ypes

jsinglet requested a review from a team as a code owner June 9, 2021 20:18

github-actions bot added the C++ label Jun 9, 2021

rdmarsh2 reviewed Jun 9, 2021

View reviewed changes

jsinglet added 4 commits June 9, 2021 17:11

removed accidental modification.

1fe3c9d

removed accidental modification.

b91a0db

format ql test files.

01cac13

autoformatting

14c419a

jbj reviewed Jun 10, 2021

View reviewed changes

jsinglet added 5 commits June 10, 2021 09:52

Comment changes

f174d7a

Changes to structure per feedback of @jbj

0d3f53b

comment change

bd7c416

more maintainable pattern for class abstractions

2a01324

changlog entry

219dc71

github-actions bot added the documentation label Jun 10, 2021

this should be abstract

cd61fb4

jbj previously approved these changes Jun 11, 2021

View reviewed changes

jsinglet dismissed jbj’s stale review via 9c946a7 June 11, 2021 13:49

Update cpp/change-notes/2021-06-10-std-types.md

9c946a7

Co-authored-by: Jonas Jensen <jbj@github.com>

Formatting fixes, comment moving.

8c6c011

jbj approved these changes Jun 11, 2021

View reviewed changes

jbj merged commit e23b88b into github:main Jun 11, 2021

	* A common base type for describing the C/C++ fixed-width numeric types
	* A C/C++ fixed-width numeric type, such as `int8_t`

	* A common base type for describing the C/C++ minimum-width numeric types.
	* A C/C++ minimum-width numeric type, such as `int_least8_t` or `uint_fast16_t`.

	* A common base type for describing the C/C++ maximum-width numeric types.
	* A C/C++ maximum-width numeric type, either `intmax_t` or `uintmax_t`.

	* A common base type for describing enum types that are based on fixed-width types.
	* An enum type based on a fixed-width integer type. For instance, `enum e: uint8_t = { a, b };`

		class FixedWidthIntegralType extends TFixedWidthIntegralType {
		FixedWidthIntegralType() { this instanceof TFixedWidthIntegralType }

Implementation of standard C/C++ fixed width, minimum width, and maximum width types #6052

Implementation of standard C/C++ fixed width, minimum width, and maximum width types #6052

Uh oh!

Conversation

jsinglet commented Jun 9, 2021

Uh oh!

rdmarsh2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MathiasVP Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jbj commented Jun 11, 2021

Uh oh!

Uh oh!

MathiasVP Jun 10, 2021 •

edited

Loading