Page MenuHomePhabricator

[LifetimeAnalysis] Add [[gsl::Pointer]] to llvm::StringRef
ClosedPublic

Authored by mgehre on Aug 19 2019, 2:36 PM.

Diff Detail

Event Timeline

mgehre created this revision.Aug 19 2019, 2:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2019, 2:36 PM
mgehre edited the summary of this revision. (Show Details)Aug 19 2019, 2:38 PM

I do not know if there are some build bots with -Werror that would warn for unknown attributes. In case it fails the build bots we could extend the include/llvm/Support/Compiler.h header with a macro.

gribozavr accepted this revision.Aug 20 2019, 1:16 AM
This revision is now accepted and ready to land.Aug 20 2019, 1:16 AM

Seems like an attribute that could go on ArrayRef as well?

Generally, ArrayRef is also a candidate. But there we have the complication that OwningArrayRef indirectly inherits from ArrayRef. One is a gsl::Owner, the other a gsl::Pointer, and we
need to make sure that we handle it correctly when OwningArrayRef is accessed through its base class which is then seen as a Pointer.
I would thus defer this part to a later PR.

Generally, ArrayRef is also a candidate. But there we have the complication that OwningArrayRef indirectly inherits from ArrayRef. One is a gsl::Owner, the other a gsl::Pointer, and we
need to make sure that we handle it correctly when OwningArrayRef is accessed through its base class which is then seen as a Pointer.
I would thus defer this part to a later PR.

Can OwningArrayRef be changed not to inherit from ArrayRef?

This revision was automatically updated to reflect the committed changes.
bondhugula added a subscriber: bondhugula.EditedApr 16 2020, 1:10 AM

This revision leads to a pervasive warning from StringRef.h (on the release build with Clang/Clang++ 9.0.1):

llvm/include/llvm/ADT/StringRef.h:57:11: warning: unknown attribute 'Pointer' ignored [-Wunknown-attributes]

This revision leads a pervasive warning from StringRef.h (on the release build with Clang/Clang++ 9.0.1):

llvm/include/llvm/ADT/StringRef.h:57:11: warning: unknown attribute 'Pointer' ignored [-Wunknown-attributes]

Same thing with GCC 7:

../include/llvm/ADT/StringRef.h:57:26: warning: ‘gsl::Pointer’ scoped attribute directive ignored [-Wattributes]
   class [[gsl::Pointer]] StringRef {
                          ^~~~~~~~~
labath added a subscriber: labath.Apr 16 2020, 2:48 AM

+1

builds with compilers which don't implement this attribute are basically useless. This should either be predicated on the compiler supporting this attribute, or -W(unknown-)attributes should be disabled in cmake.

+1

builds with compilers which don't implement this attribute are basically useless. This should either be predicated on the compiler supporting this attribute, or -W(unknown-)attributes should be disabled in cmake.

Disabling -W(unknown-)attributes will just allow this kind of issue to become more rampant, this needs adding to llvm\support\compiler.h similar to how we manage LLVM_FALLTHROUGH

I'll add it to Compiler.h

vsk added a subscriber: vsk.Apr 16 2020, 10:10 AM

Hi @mgehre, thanks for working on this. I noticed that your work appeared on GitHub at remotes/llvm/StringRefGslPointer. I think those branches are just for releases. In the future, please stage work on your personal fork.

Hi @vsk, sorry, I pushed that branch by accident to the wrong remote.