Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: fix warning for comment in embedtest #49416

Merged
merged 1 commit into from
Sep 2, 2023

Conversation

pluris
Copy link
Contributor

@pluris pluris commented Aug 31, 2023

  • Build log(Ubuntu 20.04)
...
../test/embedding/embedtest.cc:67:3: warning: multi-line comment [-Wcomment]
   67 |   // embedtest js_code_to_eval arg1 arg2... \
      |   ^
../test/embedding/embedtest.cc:72:3: warning: multi-line comment [-Wcomment]
   72 |   // embedtest --embedder-snapshot-blob blob-path \
...

Warning messages are occurring due to a comments on the following line.

// Format of the arguments of this binary:
// Building snapshot:
// embedtest js_code_to_eval arg1 arg2... \
// --embedder-snapshot-blob blob-path \
// --embedder-snapshot-create
// [--embedder-snapshot-as-file]
// Running snapshot:
// embedtest --embedder-snapshot-blob blob-path \
// [--embedder-snapshot-as-file]
// arg1 arg2...
// No snapshot:
// embedtest arg1 arg2...

A line of comment starting with // is supposed to end at the newline character.
However, if a backslash(\) is placed at the end of the line, the comment is extended to the next line. This is because the backslash at the end of a line is a line-continuation character, which tells the compiler to treat the next line as a continuation of the current line. This can lead to unexpected behavior, hence the compiler warning.

There are several suggestions for solving this, but I suggest a simple way to remove the backslash(\).

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Aug 31, 2023
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2023
@nodejs-github-bot
Copy link
Collaborator

@deokjinkim deokjinkim added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 2, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 2, 2023
@nodejs-github-bot nodejs-github-bot merged commit ea32c3c into nodejs:main Sep 2, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in ea32c3c

UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
PR-URL: #49416
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Deokjin Kim <deokjin81.kim@gmail.com>
@UlisesGascon UlisesGascon mentioned this pull request Sep 10, 2023
@pluris pluris deleted the fix/warning_embedtest branch September 11, 2023 00:46
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
PR-URL: nodejs#49416
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Deokjin Kim <deokjin81.kim@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants