Bug 198079 – [WHLSL] ReadModifyWriteExpression always has a result and new value expression

• Component: WebGPU
• Status: RESOLVED FIXED

[Saam Barati]

we branch on it being there in Visitor. Seems unnecessary, since we always set it after we create a ReadModifyWriteExpression. We should probably make it a ctor parameter.

[Saam Barati]

Same with newValueExpression

[Saam Barati]

Spoke with Myles. We should assert it's non-null. We can't make it an argument to ctor though because of some circular references.

[Saam Barati]

Created attachment 370533 [details]
patch

[Myles C. Maxfield]

Comment on attachment 370533 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370533&action=review

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLReadModifyWriteExpression.h:90
> -    Expression* newValueExpression() { return m_newValueExpression ? &*m_newValueExpression : nullptr; }
> -    Expression* resultExpression() { return m_resultExpression ? &*m_resultExpression : nullptr; }
> +    Expression& newValueExpression() { ASSERT(m_newValueExpression); return *m_newValueExpression; }
> +    Expression& resultExpression() { ASSERT(m_resultExpression); return *m_resultExpression; }

I think it's better style to break this up into two lines

Expression& newValueExpression()
{
    ASSERT(m_newValueExpression);
    return *m_newValueExpression;
}

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLReadModifyWriteExpression.h:97
> +        m_newValueExpression.reset();

Is this line necessary?

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLReadModifyWriteExpression.h:103
> +        m_resultExpression.reset();

Is this line necessary?

[Saam Barati]

Comment on attachment 370533 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370533&action=review

Thanks for the review.

>> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLReadModifyWriteExpression.h:90
>> +    Expression& resultExpression() { ASSERT(m_resultExpression); return *m_resultExpression; }
> 
> I think it's better style to break this up into two lines
> 
> Expression& newValueExpression()
> {
>     ASSERT(m_newValueExpression);
>     return *m_newValueExpression;
> }

Will do

>> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLReadModifyWriteExpression.h:97
>> +        m_newValueExpression.reset();
> 
> Is this line necessary?

Strictly speaking, no, given the context this is called in. However, it felt a bit odd to leave the optional engaged even after moving the contents of a UniqueRef out of it.

[Saam Barati]

Created attachment 370567 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 370567 [details]
patch for landing

Clearing flags on attachment: 370567

Committed r245745: <https://trac.webkit.org/changeset/245745>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51099940>

[Myles C. Maxfield]

*** Bug 198170 has been marked as a duplicate of this bug. ***