Page MenuHomePhabricator

[clang-tidy] Fix crash in readability-redundant-string-cstr
ClosedPublic

Authored by njames93 on Mar 25 2020, 3:45 AM.

Diff Detail

Event Timeline

njames93 created this revision.Mar 25 2020, 3:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2020, 3:45 AM
njames93 edited the summary of this revision. (Show Details)Mar 25 2020, 3:47 AM
njames93 added a project: Restricted Project.

Could you provide more information about why these null checks are needed in this case?

Could you provide more information about why these null checks are needed in this case?

Well the fact it was crashing without those null checks was the reason they were added. I'm not 100% sure of the root cause and this could just be a case of applying a plaster to a broken arm. I'm guessing the root cause of the crash is the fact the callee is a CXXMethodDecl which is not being handled but I honestly don't know.

Could you provide more information about why these null checks are needed in this case?

Well the fact it was crashing without those null checks was the reason they were added. I'm not 100% sure of the root cause and this could just be a case of applying a plaster to a broken arm. I'm guessing the root cause of the crash is the fact the callee is a CXXMethodDecl which is not being handled but I honestly don't know.

Right -- what I meant is a more detailed description of why, for example, tryGetCallExprAncestorForCxxConstructExpr can't find the CallExpr in this case -- is it not there, or does it not have the expected shape, or something else? What does the AST look like?

I'm worried about adding defensive checks because they can make code more difficult to fix in future.

njames93 updated this revision to Diff 252646.Mar 25 2020, 12:22 PM
  • Fixed by replacing cumbersome code with the arguably proper solution

Right -- what I meant is a more detailed description of why, for example, tryGetCallExprAncestorForCxxConstructExpr can't find the CallExpr in this case -- is it not there, or does it not have the expected shape, or something else? What does the AST look like?

I'm worried about adding defensive checks because they can make code more difficult to fix in future.

To be honest the whole fix that caused the crash in the first place was a mess, I have decided to fix this by checking if the parent is a temporary expr bound to an r value, as well as removing the hacky looking code from the previous patch.

gribozavr2 accepted this revision.Mar 30 2020, 4:45 PM
This revision is now accepted and ready to land.Mar 30 2020, 4:45 PM
This revision was automatically updated to reflect the committed changes.