Fix dead store in `uf` variable #167

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@charliesome
Contributor

charliesome commented Jan 17, 2014

This popped up when running clang-analyzer over http-parser:

http_parser.c:2122:3: warning: Value stored to 'uf' is never read
  uf = old_uf = UF_MAX;
  ^    ~~~~~~~~~~~~~~~
http_parser.c
@@ -99,7 +99,7 @@
FOR##_mark = NULL; \
} \
} while (0)
-
+

This comment has been minimized.

Show comment Hide comment
@indutny

indutny Jan 17, 2014

Member

Unrelated change, please remove.

@indutny

indutny Jan 17, 2014

Member

Unrelated change, please remove.

@indutny

This comment has been minimized.

Show comment Hide comment
@indutny

indutny Jan 17, 2014

Member

One small style nit, otherwise LGTM. Seems like it is really set in all possible cases.

Member

indutny commented Jan 17, 2014

One small style nit, otherwise LGTM. Seems like it is really set in all possible cases.

Fix dead store in `uf` variable
http_parser.c:2122:3: warning: Value stored to 'uf' is never read
  uf = old_uf = UF_MAX;
  ^    ~~~~~~~~~~~~~~~
@charliesome

This comment has been minimized.

Show comment Hide comment
@charliesome

charliesome Jan 17, 2014

Contributor

@indutny Fixed

Contributor

charliesome commented Jan 17, 2014

@indutny Fixed

@indutny

This comment has been minimized.

Show comment Hide comment
@indutny

indutny Jan 17, 2014

Member

One last thing, could you please sign CLA?

Member

indutny commented Jan 17, 2014

One last thing, could you please sign CLA?

@indutny

This comment has been minimized.

Show comment Hide comment
@indutny

indutny Jan 17, 2014

Member

It is there, just in case: http://nodejs.org/cla.html

Member

indutny commented Jan 17, 2014

It is there, just in case: http://nodejs.org/cla.html

@charliesome

This comment has been minimized.

Show comment Hide comment
@charliesome

charliesome Jan 18, 2014

Contributor

It's a 5 char patch, signing a CLA seems a bit over the top.

Contributor

charliesome commented Jan 18, 2014

It's a 5 char patch, signing a CLA seems a bit over the top.

@indutny

This comment has been minimized.

Show comment Hide comment
@indutny

indutny Jan 18, 2014

Member

@charliesome sorry, I am not the one who invented that rules, I'm just the one who should follow them.

Member

indutny commented Jan 18, 2014

@charliesome sorry, I am not the one who invented that rules, I'm just the one who should follow them.

@charliesome

This comment has been minimized.

Show comment Hide comment
@charliesome

charliesome Jan 23, 2014

Contributor

@indutny This patch is just a deletion, it does not add any new code. This is not a copyrightable contribution, therefore a CLA is unnecessary.

Contributor

charliesome commented Jan 23, 2014

@indutny This patch is just a deletion, it does not add any new code. This is not a copyrightable contribution, therefore a CLA is unnecessary.

@tjfontaine

This comment has been minimized.

Show comment Hide comment
@tjfontaine

tjfontaine Jan 23, 2014

Contributor

It's far easier to just apply the logic regardless of the class of
contribution. Having that consistency makes it convenient for us to
make sure all contributions comply, and opens the door for you to
contribute later. Sorry if this is inconvenient for you or
incompatible with your employment contract, it's a necessary course of
action for us.

Contributor

tjfontaine commented Jan 23, 2014

It's far easier to just apply the logic regardless of the class of
contribution. Having that consistency makes it convenient for us to
make sure all contributions comply, and opens the door for you to
contribute later. Sorry if this is inconvenient for you or
incompatible with your employment contract, it's a necessary course of
action for us.

@charliesome charliesome deleted the charliesome:fix-clang-analyzer-warning branch Jan 23, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment