src: replace typedef with using by RaisinTen · Pull Request #38228 · nodejs/node

RaisinTen · 2021-04-13T15:24:03Z

No description provided.

addaleax

I would lean towards not doing this, given that this will give us merge conflicts and little benefit in return

addaleax · 2021-04-13T21:57:18Z

    kSignPrivateKey,
    kSignPublicKey,
    kSignMalformedSignature
-  } Error;


This doesn't need to be typedef or using

@addaleax I think we should we turn this into an enum class. A lot of its usage in crypto_sig.cc seems to precede the enumerations with Error:: except for a few instances.

Yeah, both enum class or plain enum seem fine here

addaleax · 2021-04-13T21:57:34Z


 #if !defined __cplusplus || (defined(_MSC_VER) && _MSC_VER < 1900)
-    typedef uint16_t char16_t;
+    using char16_t = uint16_t;


This is a C header

addaleax · 2021-04-13T21:58:11Z

 #pragma D depends_on library procfs.d

-typedef struct {
+using node_dtrace_connection_t = struct {


This is neither C nor C++ ... maybe keep this as-is?

addaleax · 2021-04-13T21:58:35Z

  void* nm_priv;
  void* reserved[4];
-} napi_module;
+};


Ditto, C header

RaisinTen · 2021-04-14T14:18:59Z

@addaleax I made this change because using allows us to do whatever typedef does and it additionally also allows this which typedefs can't (unless we are using an enclosing struct):

node/src/base_object.h

Lines 272 to 273 in 3377eb9

    
           template <typename T> 
        
           using BaseObjectPtr = BaseObjectPtrImpl<T, false>;

Should we still not go ahead with this PR?

addaleax · 2021-04-14T14:35:42Z

@RaisinTen I mean, that’s a good motivation for using using in new code, but in the places where we currently have typedefs they work just fine. It’s not a terrible thing to change them, but the main effect it will have will still be just giving merge conflicts.

RaisinTen · 2021-04-14T14:52:04Z

@addaleax I think most of the PRs are functionality changes that don't cover parts of the code where aliases are declared. So will landing this change really have much of merge conflicts? Also, using is more readable than typedef, with the way the operands are placed around the =, which applies to all the typedefs. Could readability rule over the effect of the merge conflicts? 😛

addaleax · 2021-04-14T15:10:20Z

@RaisinTen I’m not blocking this :) I’d suggest squashing this down into a single commit and backporting that to the other release lines as soon as this lands, though.

legendecas

The headers of node-api like js_native_api_types.h are C headers and should not use C++ features in it. Just marking the PR request for changes so that it will not be landed by accident. Please feel free to dismiss if updates are submitted regarding node-api part.

jasnell · 2021-04-15T13:25:01Z

I'm generally -1 on changing all of these in bulk like this. I generally will only switch to using from typedef if there are other larger reflectors happening. As @addaleax mentions, doing it like this will cause pain with little benefit

RaisinTen · 2021-04-15T14:43:48Z

Thank you for the reviews. :)
Closing as this change does not seem to do much other than creating more merge conflicts.

RaisinTen added 30 commits April 13, 2021 19:45

src: replace typedef with using in aliased_buffer.h

9fa9dc5

src: replace typedef with using in api/hooks.cc

709a808

src: replace typedef with using in crypto/crypto_cipher.h

04d7548

src: replace typedef with using in crypto/crypto_clienthello.h

8dfa19f

src: replace typedef with using in crypto/crypto_ec.cc

43f20af

src: replace typedef with using in crypto/crypto_groups.h

f22484a

src: replace typedef with using in crypto/crypto_keys.cc

6125364

src: replace typedef with using in crypto/crypto_sig.h

f443f29

src: replace typedef with using in crypto/crypto_tls.h

3c039ec

src: replace typedef with using in env.h

d935dd2

src: replace typedef with using in inspector_socket.cc

822f8ba

src: replace typedef with using in js_native_api_types.h

d9070f3

src: replace typedef with using in js_native_api_v8.h

bf726c8

src: replace typedef with using in memory_tracker.h

aea7475

src: replace typedef with using in node.cc

8a48480

src: replace typedef with using in node.d

9dc9b0f

src: replace typedef with using in node.h

7a6dba4

src: replace typedef with using in node_api.cc

47b70e9

src: replace typedef with using in node_api.h

a7f5ed5

src: replace typedef with using in node_api_types.h

749d6fb

src: replace typedef with using in node_buffer.h

389913d

src: replace typedef with using in node_dtrace.h

ff8cd8e

src: replace typedef with using in node_file.cc

aed8988

src: replace typedef with using in node_file.h

7f4b252

src: replace typedef with using in node_http2.h

64091ba

src: replace typedef with using in node_http_common.h

e61f221

src: replace typedef with using in node_http_parser.cc

78ab5f4

src: replace typedef with using node_i18n.cc

ca1e29f

src: replace typedef with using node_internals.h

a98cc40

src: replace typedef with using node_messaging.h

6dc535d

RaisinTen added 13 commits April 13, 2021 20:34

src: replace typedef with using in node_options.h

bbcc47f

src: replace typedef with using in node_platform.h

ab4e7b2

src: replace typedef with using in node_process_methods.cc

c3898c4

src: replace typedef with using in node_provider.d

9f52478

src: replace typedef with using in node_win32_etw_provider.h

f419f2d

src: replace typedef with using in node_zlib.cc

fa0b609

src: replace typedef with using in req_wrap.h

0def98a

src: replace typedef with using in stream_wrap.cc

e76be66

src: replace typedef with using in string_bytes.cc

3fe9780

src: replace typedef with using in string_search.h

e13cbf2

src: replace typedef with using in tcp_wrap.h

dd23533

src: replace typedef with using in udp_wrap.h

6149c9c

src: replace typedef with using in util.h

3da92f8

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Apr 13, 2021

addaleax reviewed Apr 13, 2021

View reviewed changes

legendecas requested changes Apr 15, 2021

View reviewed changes

RaisinTen closed this Apr 15, 2021

RaisinTen deleted the src/replace-typedef-with-using branch April 15, 2021 14:44

Uh oh!

Conversation

RaisinTen commented Apr 13, 2021

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

RaisinTen Apr 14, 2021

Choose a reason for hiding this comment

Uh oh!

addaleax Apr 14, 2021

Choose a reason for hiding this comment

Uh oh!

addaleax Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

addaleax Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

addaleax Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

RaisinTen commented Apr 14, 2021

Uh oh!

addaleax commented Apr 14, 2021

Uh oh!

RaisinTen commented Apr 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Apr 14, 2021

Uh oh!

legendecas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 15, 2021

Uh oh!

RaisinTen commented Apr 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

RaisinTen commented Apr 14, 2021 •

edited

Loading

legendecas left a comment •

edited

Loading