Summary: | Update the way generate-xcfilelists returns strings from functions | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keith Rollin <krollin> | ||||||||
Component: | Tools / Tests | Assignee: | Keith Rollin <krollin> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, commit-queue, dino, lforschler, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 195977 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Keith Rollin
2019-03-19 16:24:10 PDT
Created attachment 365258 [details]
Patch
Created attachment 365282 [details]
Patch
Comment on attachment 365282 [details]
Patch
Patch got borked.
Created attachment 365307 [details]
Patch
Comment on attachment 365307 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365307&action=review > Tools/ChangeLog:31 > + The solution to this is to use a different idiom for returning > + strings. The one we now is to pass in the name of the variable to > + receive the string result: I probably shouldn't review this patch because I don't even see where exactly we spawn shells to call a function, but I can't help myself asking whether executing sub-shells is unavoidable in the first place. > Tools/Scripts/generate-xcfilelists:612 > + [[ "${GX_PROVISIONAL_CONFIGURATION}" == "" ]] && { eval $GX_RESULT=""; return; } I can't find any such rules in WebKit coding style, but I remember seeing reviewer feedback about how we don't do formatting of this kind in WebKit as a matter of coding style. The rationale is that it makes refactoring harder, and I agree with that. (In reply to Alexey Proskuryakov from comment #6) > Comment on attachment 365307 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=365307&action=review > > > Tools/ChangeLog:31 > > + The solution to this is to use a different idiom for returning > > + strings. The one we now is to pass in the name of the variable to > > + receive the string result: > > I probably shouldn't review this patch because I don't even see where > exactly we spawn shells to call a function, I don't know who's good for reviewing bash scripts either, but I'll add Dean, who looked at this script before and understood it enough to hack it up. > but I can't help myself asking > whether executing sub-shells is unavoidable in the first place. A fair question. The normal idiom for having a shell function return a string and have that string be assigned to a variable is: FOO=$(some_function) This syntax executes "some_function" in a sub-shell and replaces the whole $(...) with the textual output of that function, allowing that text to be assigned to the variable. You can see this in the actual script in places like: local GX_SDK=$(get_canonical_platform_name "$1") There's no replaceable construct that can replace that, AFAIK. For instance, we can't say: local GX_SDK=get_canonical_platform_name "$1" because that won't treat "get_canonical_platform_name $1" as a function call. We can't use as something that allows for functions to be called and the output gathered in a single location: local GX_SDK={get_canonical_platform_name "$1";} because that's not valid syntax. Searching the web for alternatives to $(...), I didn't find any solution that I liked better than what I ended up using here. > > Tools/Scripts/generate-xcfilelists:612 > > + [[ "${GX_PROVISIONAL_CONFIGURATION}" == "" ]] && { eval $GX_RESULT=""; return; } > > I can't find any such rules in WebKit coding style, but I remember seeing > reviewer feedback about how we don't do formatting of this kind in WebKit as > a matter of coding style. The rationale is that it makes refactoring harder, > and I agree with that. I like the spacing because it makes it really clear what the function is returning. All of the strings I'm returning are lined up, making it clear what's happening in what situations. Without that spacing, I had a hard time seeing what was happening. So, from that aspect of readability and maintainability, I like it. What refactoring do you see it interfering with? About all I can think if is if I add some new cases that require the amount of spacing to be increased, meaning that I'd be touching lines for no reason other than to increase that spacing. For me, the maintainability of seeing what the code is doing outweighs that. > What refactoring do you see it interfering with?
The general example is find and replace.
Comment on attachment 365307 [details] Patch Clearing flags on attachment: 365307 Committed r243527: <https://trac.webkit.org/changeset/243527> All reviewed patches have been landed. Closing bug. |