Open Bug 1208262 Opened 9 years ago Updated 2 years ago

Integrate the C++ Guidelines Support Library (GSL) into the tree

Categories

(Core :: MFBT, defect)

Product:
Core
Component:
MFBT
Type:
defect

Tracking

()

People

(Reporter: jwatt, Unassigned)

References

Details

To support:

  https://github.com/isocpp/CppCoreGuidelines

there's an MIT licensed library called  Guidelines Support Library (GSL) that apparently works in C++11 supporting versions of Clang, GCC and MS compilers:

  https://github.com/Microsoft/GSL/

We should consider integrating this lib into the tree.
For those who aren't aware of this stuff, here's the CppCon talk where Bjarne Stroustrup talks about it:

  https://www.youtube.com/watch?t=9&v=1OEu9C51K2A
GSL and the CPP Guidelines assume that |std::unique_ptr|, |std::shared_ptr|, etc. exist and work. It would be great if, as a preliminary step, |std::unique_ptr| and |std::shared_ptr| were made available on all Gecko platforms. Until then, GSL seems like a non-starter.

Does the GSL really make sense in a context where exceptions are not allowed? I looked at |array_view|, for example, and it seems like it is a worse interface for an exception-less codebase than, for example, |mozilla::pkix::Input|/|mozilla::pkix::Reader| [1], which were optimized for exception-less codebases by returning error codes when necessary instead of just aborting the process or throwing an exception. See also bug 1188957, where the error reporting will be made more generic so that |mozilla::pkix::Input| and |mozilla::pkix::Reader| can be used outside of |mozilla::pkix|. So even if GSL were available in Gecko, it doesn't seem like it would even be the best choice within the Mozilla codebase.

[1] https://github.com/briansmith/mozillapkix/blob/master/include/pkix/Input.h
GSL also assumes C++14, which we currently use, and therefore cannot be checked in as is.
(FWIW I am doubtful that we'll ever want to import GSL proper because of practical complications of doing so such as dealing with various C++ standard libraries we have to support, and using C++14, etc.  My plan was to borrow the ideas and the parts that we can use.)
Summary: Integrate the Guidelines Support Library (GSL) into the tree → Integrate the C++ Guidelines Support Library (GSL) into the tree
Version: 37 Branch → unspecified
Thanks for modernizing our C++ code base.

FYI there's also https://github.com/martinmoene/gsl-lite for compilers before C++14.

In any case it would be great to adopt some of the ideas of GSL. |not_null| and |array_view| look like low-hanging fruits. I'd also like to have equivalents to |make_unique| and |make_shared| for our smart pointers.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #5)
> Thanks for modernizing our C++ code base.
> 
> FYI there's also https://github.com/martinmoene/gsl-lite for compilers
> before C++14.

Cool, this may be a more suitable candidate.  I'll have a look.

> In any case it would be great to adopt some of the ideas of GSL. |not_null|
> and |array_view| look like low-hanging fruits. I'd also like to have
> equivalents to |make_unique| and |make_shared| for our smart pointers.

We have MakeUnique, FWIW.
(In reply to Ehsan Akhgari (don't ask for review please) from comment #6)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #5)
> > Thanks for modernizing our C++ code base.
> > 
> > FYI there's also https://github.com/martinmoene/gsl-lite for compilers
> > before C++14.
> 
> Cool, this may be a more suitable candidate.  I'll have a look.
> 
> > In any case it would be great to adopt some of the ideas of GSL. |not_null|
> > and |array_view| look like low-hanging fruits. I'd also like to have
> > equivalents to |make_unique| and |make_shared| for our smart pointers.
> 
> We have MakeUnique, FWIW.

Adding MakeRefPtr would be pretty easy too, if we wanted it. We also have OwningNonNull which is kinda like NotNull I think (don't know too much about the others).

ArrayView sounds nice, it's kinda like taking some of the fancy ns*Strings and making them generic over all array types, which could be neat.
(In reply to Michael Layzell [:mystor] from comment #7)
> Adding MakeRefPtr would be pretty easy too, if we wanted it.

We have MakeAndAddRef (bikeshedding welcome on the name).
(In reply to Nathan Froyd [:froydnj] from comment #8)
> (In reply to Michael Layzell [:mystor] from comment #7)
> > Adding MakeRefPtr would be pretty easy too, if we wanted it.
> 
> We have MakeAndAddRef (bikeshedding welcome on the name).

MakeAndAddRef returns already_AddRefed, so the name makes sense. MakeRefPtr could return RefPtr instead.

But first it would be interesting to compare the two in real situations, see if the distinction is useful depending on usage, or if one is clearly always better.
(In reply to Gerald Squelart [:gerald] from comment #9)
> (In reply to Nathan Froyd [:froydnj] from comment #8)
> > (In reply to Michael Layzell [:mystor] from comment #7)
> > > Adding MakeRefPtr would be pretty easy too, if we wanted it.
> > 
> > We have MakeAndAddRef (bikeshedding welcome on the name).
> 
> MakeAndAddRef returns already_AddRefed, so the name makes sense. MakeRefPtr
> could return RefPtr instead.
> 
> But first it would be interesting to compare the two in real situations, see
> if the distinction is useful depending on usage, or if one is clearly always
> better.

IIUC, things like MakeRefPtr and MakeUnique are designed to be used with auto, so that we can write e.g.
> auto ptr = MakeRefPtr<Class>(...);
and avoid a long redundant type name. But AFAIK, some reviewers really don't like `auto`, and they may consider things like MakeRefPtr confused.

With MakeAndAddRef in this case, we would need to write:
> RefPtr<Class> ptr = MakeAndAddRef<Class>(...);

I guess MakeAndAddRef is more useful for passing an already_AddRefed to some function which accepts already_AddRefed&&, but that probably could be covered with MakeRefPtr<Class>(...).forget() as well.

So probably we need only MakeRefPtr. The only question is whether reviewers are happy with the first usage.
Interesting discussion here. :) Adding my 2cts:

|MakeRefPtr|, |MakeCOMPtr|, |MakeAutoPtr|, etc. seem like good names to me. The functions would return the respective smart pointers. With inlining and move semantics, this has zero overhead. Any explicit calls of |new| or |delete| would be considered 'incorrect code'. This would eliminate potential resource leaks.

I've see |OwningNonNull| before, but it's only for smart pointers. I thought about generalizing it to |NonNull| for raw pointers, but I didn't have time and a specific use case yet.

My general use case for |NonNull| would be interfaces. Instead of

  void f(int* aPtr)
  {
    MOZ_ASSERT(aPtr);
    ...actual code...
  }

the code would look like

  void f(NonNull<int*> aPtr)
  {
    ...actual code...
  }

The non-null invariant is specified in the interface. It's a lot harder to use it incorrectly or break the code during refactoring.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #11)
> I've see |OwningNonNull| before, but it's only for smart pointers. I thought
> about generalizing it to |NonNull| for raw pointers, but I didn't have time
> and a specific use case yet.

clang has a _Nonnull pointer type specifier that may be useful here. As a type specifier, it is easier to use than gcc's __attribute__((nonnull)) or __attribute__((returns_nonnull) function attributes:

  int clang_example(int * _Nonnull ptr) { return *ptr; }
  int gcc_example(int * ptr) __attribute__((nonnull(1))) { return *ptr; }

http://clang.llvm.org/docs/AttributeReference.html#nullability-attributes
Assignee: nobody → ehsan
I'm circling around and experimenting with not_null (well, NotNull) in bug 1266651. Also MaybeNull -- even though it's apparently been removed from GSL -- because I think it's useful.
Depends on: 1272203
(In reply to Nicholas Nethercote [:njn] from comment #13)
> I'm circling around and experimenting with not_null (well, NotNull)

Now that this bug has stalled but NotNull shows we might do this stuff piecewise in MFBT, I filed bug 1295611 about (one-dimensional) span.
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1295611
(In reply to Henri Sivonen (:hsivonen) from comment #14)
> Now that this bug has stalled but NotNull shows we might do this stuff
> piecewise in MFBT, I filed bug 1295611 about (one-dimensional) span.

Now we have both NotNull and Span (currently on inbound). Should we close this bug as WONTFIX?
Assignee: ehsan → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.