Cpplint produces false positives for FastApiOptions

[anonrig]

Following line is a perfectly fine declaration for v8 fast API.

using CFunctionCallbackWithInput =
    void (*)(v8::Local<v8::Value> receiver,
             const v8::FastOneByteString& source,
             const v8::FastApiTypedArray<uint8_t>& destination,
             const v8::FastApiTypedArray<uint32_t>& result,
             v8::FastApiCallbackOptions& options);

But it produces the following error

Running C++ linter...
src/node_encoding.cc:101:  Is this a non-const reference? If so, make const or use a pointer: FastApiCallbackOptions& options  [runtime/references] [2]
Done processing src/node_encoding.cc
src/node_external_reference.h:20:  Is this a non-const reference? If so, make const or use a pointer: v8::FastApiCallbackOptions& options  [runtime/references] [2]
Done processing src/node_external_reference.h
Total errors found: 2
make: *** [tools/.cpplintstamp] Error 1

[richardlau]

Upstream cpplint is https://github.com/cpplint/cpplint. You can check if there's a newer version that fixes that and update the version in this repository if so (although I think we do float some Node.js specific patches on top of it).

[anonrig]

I think this can be fixed with updating our already patched cpplint implementation through the following line:

  # We allow non-const references in a few standard places, like functions
  # called "swap()" or iostream operators like "<<" or ">>".  Do not check
  # those function parameters.
  #
  # We also accept & in static_assert, which looks like a function but
  # it's actually a declaration expression.
  allowed_functions = (r'(?:[sS]wap(?:<\w:+>)?|'
                           r'operator\s*[<>][<>]|'
                           r'static_assert|COMPILE_ASSERT'
                           r')\s*\(')

[vivek378521]

Hey @anonrig I want to take a go at this. Modifying the above regex to the below one should fix this, but can there be a more generic regex? A nudge in the right direction will be appreciated.

allowed_functions = (r'(?:[sS]wap(?:<\w:+>)?|' r'operator\s*[<>][<>]|' r'static_assert|COMPILE_ASSERT' r'FastApiCallbackOptions' r')\s*\(')

[shubham9411]

Hi @anonrig, There is a rule in cpplint.py for non-const reference which is triggering this error. I see it is also added in contributing guide here https://github.com/nodejs/node/blob/97720652317411f8647efde4c1d1d9ba36fa7ed9/doc/contributing/cpp-style-guide.md#avoid-non-const-references. We have a def CheckForNonConstReference in lint file to check for these. Do we want to skip this error for FastApiCallbackOptions only?

[ebouye]

@anonrig if we update patched cpplint it makes https://github.com/nodejs/node/blob/main/doc/contributing/cpp-style-guide.md#avoid-non-const-references not valid anymore, right ?

[ebouye]

@anonrig this issue is still relevant ?