Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WGSL] Use String instead of StringView for Identifiers #8697

Merged
merged 1 commit into from
Jan 18, 2023
Merged

[WGSL] Use String instead of StringView for Identifiers #8697

merged 1 commit into from
Jan 18, 2023

Conversation

tadeuzagallo
Copy link
Member

@tadeuzagallo tadeuzagallo commented Jan 16, 2023 •
edited by webkit-early-warning-system

b965bbc

[WGSL] Use String instead of StringView for Identifiers
https://bugs.webkit.org/show_bug.cgi?id=250680
<rdar://problem/104299588>

Reviewed by Myles C. Maxfield.

There should be no extra cost if we create the string without copying and it
allows us to use runtime-generated strings which will be necessary soon.

* Source/WebGPU/WGSL/AST/ASTArrayAccess.h:
* Source/WebGPU/WGSL/AST/ASTAttribute.h:
* Source/WebGPU/WGSL/AST/ASTBuiltinAttribute.h:
* Source/WebGPU/WGSL/AST/ASTFunctionDecl.h:
* Source/WebGPU/WGSL/AST/ASTGlobalDirective.h:
(WGSL::AST::GlobalDirective::GlobalDirective):
(WGSL::AST::GlobalDirective::name const):
* Source/WebGPU/WGSL/AST/ASTIdentifierExpression.h:
* Source/WebGPU/WGSL/AST/ASTLocationAttribute.h:
* Source/WebGPU/WGSL/AST/ASTStructureAccess.h:
* Source/WebGPU/WGSL/AST/ASTStructureDecl.h:
* Source/WebGPU/WGSL/AST/ASTTypeDecl.h:
(WGSL::AST::ParameterizedType::stringToKind):
(WGSL::AST::ParameterizedType::stringViewToKind): Deleted.
* Source/WebGPU/WGSL/AST/ASTVariableDecl.h:
* Source/WebGPU/WGSL/Lexer.cpp:
(WGSL::Lexer<T>::lex):
* Source/WebGPU/WGSL/Lexer.h:
(WGSL::Lexer::makeIdentifierToken):
* Source/WebGPU/WGSL/Parser.cpp:
(WGSL::Parser<Lexer>::parseTypeDecl):
(WGSL::Parser<Lexer>::parseTypeDeclAfterIdentifier):
* Source/WebGPU/WGSL/ParserPrivate.h:
* Source/WebGPU/WGSL/Token.h:
(WGSL::Token::Token):
(WGSL::Token::operator=):
(WGSL::Token::~Token):

Canonical link: https://commits.webkit.org/259044@main

6faee3c

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl   🧪 ios-wk2 ✅ 🧪 api-mac ❌ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ❌ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge ✅ 🛠 watch-sim

@tadeuzagallo tadeuzagallo self-assigned this Jan 16, 2023
@tadeuzagallo tadeuzagallo added the WebGPU For bugs in WebGPU label Jan 16, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 532faa5)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ❌ 🧪 api-mac ✅ 🧪 gtk-wk2
❌ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 watch-sim

@tadeuzagallo tadeuzagallo marked this pull request as draft January 16, 2023 16:10
@tadeuzagallo tadeuzagallo force-pushed the eng/WGSL-Use-String-instead-of-StringView-for-Identifiers branch from 532faa5 to acb2d07 Compare January 17, 2023 09:22
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash acb2d07)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 watch-sim

@tadeuzagallo tadeuzagallo marked this pull request as ready for review January 17, 2023 09:25
litherum
litherum approved these changes Jan 17, 2023
View reviewed changes
Source/WebGPU/WGSL/AST/ASTBuiltinAttribute.h Outdated
@@ -33,17 +33,17 @@ class BuiltinAttribute final : public Attribute {
WTF_MAKE_FAST_ALLOCATED;

public:
BuiltinAttribute(SourceSpan span, StringView name)
BuiltinAttribute(SourceSpan span, String name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're doing a double copy here; surely you want the parameter to be const String&? Or you could keep the parameter a StringView but still change the parameter to String m_name like you're doing below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair, I didn't worry too much since the underlying string isn't being copied anyway, so it's equivalent to the StringView code (right?). But in any case, no downside in making it const String&.

Source/WebGPU/WGSL/AST/ASTFunctionDecl.h Outdated
@@ -66,7 +66,7 @@ class FunctionDecl final : public Decl {
public:
using List = UniqueRefVector<FunctionDecl>;

FunctionDecl(SourceSpan sourceSpan, StringView name, Parameter::List&& parameters, std::unique_ptr<TypeDecl>&& returnType, CompoundStatement&& body, Attribute::List&& attributes, Attribute::List&& returnAttributes)
FunctionDecl(SourceSpan sourceSpan, String name, Parameter::List&& parameters, std::unique_ptr<TypeDecl>&& returnType, CompoundStatement&& body, Attribute::List&& attributes, Attribute::List&& returnAttributes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

Source/WebGPU/WGSL/AST/ASTIdentifierExpression.h Outdated

namespace WGSL::AST {

class IdentifierExpression final : public Expression {
WTF_MAKE_FAST_ALLOCATED;

public:
IdentifierExpression(SourceSpan span, StringView identifier)
IdentifierExpression(SourceSpan span, String identifier)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto, and ditto elsewhere too.

djg
djg approved these changes Jan 18, 2023
View reviewed changes
Copy link
Contributor

@djg djg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: with Myles nit fixed.

@tadeuzagallo tadeuzagallo force-pushed the eng/WGSL-Use-String-instead-of-StringView-for-Identifiers branch from acb2d07 to 82cd107 Compare January 18, 2023 14:36
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 82cd107)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ❌ 🛠 ios ❌ 🛠 mac ✅ 🛠 wpe
❌ 🛠 ios-sim ❌ 🛠 mac-AS-debug ✅ 🛠 gtk ⏳ 🛠 wincairo
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ❌ 🧪 api-mac   🧪 gtk-wk2
❌ 🧪 api-ios ❌ 🧪 mac-wk1 ✅ 🧪 api-gtk
❌ 🛠 tv ❌ 🧪 mac-wk2
❌ 🛠 tv-sim ❌ 🧪 mac-AS-debug-wk2
❌ 🛠 watch ❌ 🧪 mac-wk2-stress
❌ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 18, 2023
tadeuzagallo
tadeuzagallo commented Jan 18, 2023
View reviewed changes
Source/WebGPU/WGSL/AST/ASTFunctionDecl.h Outdated
@@ -41,7 +41,7 @@ class Parameter final : public Node {
public:
using List = UniqueRefVector<Parameter>;

Parameter(SourceSpan span, StringView name, UniqueRef<TypeDecl>&& type, Attribute::List&& attributes)
Parameter(SourceSpan span, const String& name, UniqueRef<TypeDecl>&& type, Attribute::List&& attributes)
: Node(span)
, m_name(WTFMove(name))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, forgot this WTFMove here and now name is a constant. So weird that local builds aren't failing though...

@tadeuzagallo tadeuzagallo force-pushed the eng/WGSL-Use-String-instead-of-StringView-for-Identifiers branch from 82cd107 to 6faee3c Compare January 18, 2023 15:04
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 6faee3c)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl   🧪 ios-wk2 ✅ 🧪 api-mac ❌ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ❌ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge ✅ 🛠 watch-sim

@tadeuzagallo tadeuzagallo added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged merge-queue Applied to send a pull request to merge-queue labels Jan 18, 2023
@tadeuzagallo
[WGSL] Use String instead of StringView for Identifiers
b965bbc 
https://bugs.webkit.org/show_bug.cgi?id=250680
<rdar://problem/104299588>

Reviewed by Myles C. Maxfield.

There should be no extra cost if we create the string without copying and it
allows us to use runtime-generated strings which will be necessary soon.

* Source/WebGPU/WGSL/AST/ASTArrayAccess.h:
* Source/WebGPU/WGSL/AST/ASTAttribute.h:
* Source/WebGPU/WGSL/AST/ASTBuiltinAttribute.h:
* Source/WebGPU/WGSL/AST/ASTFunctionDecl.h:
* Source/WebGPU/WGSL/AST/ASTGlobalDirective.h:
(WGSL::AST::GlobalDirective::GlobalDirective):
(WGSL::AST::GlobalDirective::name const):
* Source/WebGPU/WGSL/AST/ASTIdentifierExpression.h:
* Source/WebGPU/WGSL/AST/ASTLocationAttribute.h:
* Source/WebGPU/WGSL/AST/ASTStructureAccess.h:
* Source/WebGPU/WGSL/AST/ASTStructureDecl.h:
* Source/WebGPU/WGSL/AST/ASTTypeDecl.h:
(WGSL::AST::ParameterizedType::stringToKind):
(WGSL::AST::ParameterizedType::stringViewToKind): Deleted.
* Source/WebGPU/WGSL/AST/ASTVariableDecl.h:
* Source/WebGPU/WGSL/Lexer.cpp:
(WGSL::Lexer<T>::lex):
* Source/WebGPU/WGSL/Lexer.h:
(WGSL::Lexer::makeIdentifierToken):
* Source/WebGPU/WGSL/Parser.cpp:
(WGSL::Parser<Lexer>::parseTypeDecl):
(WGSL::Parser<Lexer>::parseTypeDeclAfterIdentifier):
* Source/WebGPU/WGSL/ParserPrivate.h:
* Source/WebGPU/WGSL/Token.h:
(WGSL::Token::Token):
(WGSL::Token::operator=):
(WGSL::Token::~Token):

Canonical link: https://commits.webkit.org/259044@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/WGSL-Use-String-instead-of-StringView-for-Identifiers branch from 6faee3c to b965bbc Compare January 18, 2023 20:17
@webkit-commit-queue
Copy link
Collaborator

Committed 259044@main (b965bbc): https://commits.webkit.org/259044@main

Reviewed commits have been landed. Closing PR #8697 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit b965bbc into WebKit:main Jan 18, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djg djg djg approved these changes

@litherum litherum litherum approved these changes

@mwyrzykowski mwyrzykowski Awaiting requested review from mwyrzykowski mwyrzykowski is a code owner

Assignees

@tadeuzagallo tadeuzagallo

Labels
WebGPU For bugs in WebGPU
Projects
None yet
Milestone
No milestone
6 participants
@tadeuzagallo @webkit-early-warning-system @webkit-commit-queue @djg @litherum @webkit-ews-buildbot