Bug 190714 - Make generated C++ code use modern C++
Summary: Make generated C++ code use modern C++
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Other
Hardware: All All
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-10-18 08:18 PDT by Rob Buis
Modified: 2020-06-03 07:29 PDT (History)
21 users (show)

See Also:


Attachments
Patch (9.40 KB, patch)
2018-10-18 08:21 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (11.19 KB, patch)
2018-10-19 09:31 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (10.97 KB, patch)
2020-05-31 07:33 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (24.29 KB, patch)
2020-06-02 00:43 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (41.91 KB, patch)
2020-06-02 23:05 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2018-10-18 08:18:43 PDT
Make generated C++ code use modern C++
Comment 1 Rob Buis 2018-10-18 08:21:26 PDT
Created attachment 352690 [details]
Patch
Comment 2 Don Olmstead 2018-10-18 10:19:02 PDT
Nice start! Might be worth it to run some of the generated C++ through clang-tidy's modernizers and see if anything else pops out while you're at it.
Comment 3 Rob Buis 2018-10-19 08:07:17 PDT
Hi Don,

(In reply to Don Olmstead from comment #2)
> Nice start! Might be worth it to run some of the generated C++ through
> clang-tidy's modernizers and see if anything else pops out while you're at
> it.

This does not work on OS X though, right? If so I'll try to get it to run next week when I should have access to linux again.
Comment 4 Rob Buis 2018-10-19 09:31:53 PDT
Created attachment 352788 [details]
Patch
Comment 5 Rob Buis 2018-10-24 10:29:05 PDT
Hi Don,

(In reply to Rob Buis from comment #3)
> Hi Don,
> 
> (In reply to Don Olmstead from comment #2)
> > Nice start! Might be worth it to run some of the generated C++ through
> > clang-tidy's modernizers and see if anything else pops out while you're at
> > it.
> 
> This does not work on OS X though, right? If so I'll try to get it to run
> next week when I should have access to linux again.

I now have clang-tidy's modernizers working on linux. However I don't think this checks  the generated c++ code, I think because they are not included in compile_commands.json. Did you ever run into this?
Comment 6 Rob Buis 2020-05-31 07:33:08 PDT
Created attachment 400702 [details]
Patch
Comment 7 EWS Watchlist 2020-05-31 07:33:43 PDT
This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)
Comment 8 BJ Burg 2020-06-01 11:39:47 PDT
Comment on attachment 400702 [details]
Patch

Looks good so far, let me know if you'd like feedback. Please ensure you include rebaselined tests when your patch is up for review.
Comment 9 Rob Buis 2020-06-01 11:41:27 PDT
Thanks Brian! Will do, can probably update the patch later in the week.
Comment 10 Rob Buis 2020-06-01 11:42:10 PDT
(In reply to Don Olmstead from comment #2)
> Nice start! Might be worth it to run some of the generated C++ through
> clang-tidy's modernizers and see if anything else pops out while you're at
> it.

Hi Don, I rebased the patch. Given your recent work on cmake + clang-tidy, do you think above is on your TODO list?
Comment 11 Rob Buis 2020-06-02 00:43:28 PDT
Created attachment 400791 [details]
Patch
Comment 12 Rob Buis 2020-06-02 06:52:10 PDT
(In reply to Brian Burg from comment #8)
> Comment on attachment 400702 [details]
> Patch
> 
> Looks good so far, let me know if you'd like feedback. Please ensure you
> include rebaselined tests when your patch is up for review.

I did include the rebaselined tests in the latest patch.
Comment 13 EWS 2020-06-02 09:10:58 PDT
Committed r262424: <https://trac.webkit.org/changeset/262424>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400791 [details].
Comment 14 Radar WebKit Bug Importer 2020-06-02 09:11:17 PDT
<rdar://problem/63877449>
Comment 15 Jonathan Bedard 2020-06-02 13:33:40 PDT
This caused a webkitpy test regression.

https://results.webkit.org/?suite=webkitpy-tests&test=webkit.messages_unittest.HeaderTest.test_receiver_headers

(Internal testing narrowed the regression range to exactly this commit)
Comment 16 Ryan Haddad 2020-06-02 16:34:20 PDT
Reverted r262424 for reason:

Caused webkitpy test failure

Committed r262461: <https://trac.webkit.org/changeset/262461>
Comment 17 Rob Buis 2020-06-02 23:05:49 PDT
Created attachment 400899 [details]
Patch
Comment 18 Rob Buis 2020-06-03 05:35:59 PDT
(In reply to Jonathan Bedard from comment #15)
> This caused a webkitpy test regression.
> 
> https://results.webkit.org/?suite=webkitpy-tests&test=webkit.
> messages_unittest.HeaderTest.test_receiver_headers
> 
> (Internal testing narrowed the regression range to exactly this commit)

webkitpy now runs without failure again. Let me know if anything more is needed.
Comment 19 Jonathan Bedard 2020-06-03 07:12:34 PDT
Comment on attachment 400899 [details]
Patch

Let's try this again! :)
Comment 20 EWS 2020-06-03 07:29:14 PDT
Committed r262486: <https://trac.webkit.org/changeset/262486>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400899 [details].