620254 - txCoreFunctionCall::evaluate NAMESPACE_URI case falls through into POSITION case

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[timeless]

103 txCoreFunctionCall::evaluate(txIEvalContext* aContext, txAExprResult** aResult)

114     switch (mType) {
115         case COUNT:
122             return aContext->recycler()->getNumberResult(nodes->size(),
123                                                          aResult);
125         case ID:
167             return NS_OK;
168         }
169         case LAST:
171             return aContext->recycler()->getNumberResult(aContext->size(),
172                                                          aResult);
174         case LOCAL_NAME:
175         case NAME:
176         case NAMESPACE_URI:
177         {
194             switch (mType) {
195                 case LOCAL_NAME:
204                     return NS_OK;
206                 case NAMESPACE_URI:
215                     return NS_OK;
217                 case NAME:
234                     return NS_OK;
236                 default:
237                 {
this break is probably missplaced:
238                     break;
239                 }
240             }
if the code wants to avoid falling through into the following case, then the break needs to be here.
241         }

If you actually *intend* to fall through, then a comment should be inserted *here* - before the case POSITION line to tell readers (and static analysis tools) that you're intentionally falling through

242         case POSITION:
243         {
244             return aContext->recycler()->getNumberResult(aContext->position(),
245                                                          aResult);
246         }

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

As you can see by looking at the two switch statements, we can never hit the default, it's only there to silence a warning.3

Comment 2

[Daniel Veditz [:dveditz]]

Maybe throwing in a NS_NOTREACHED() in default: would be a good idea, in case someone adds a new mType in the future and forgets to update this spot?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Even adding a new mType wouldn't be a problem. Look at the two switches in comment 0

Comment 4

[Johnny Stenback (:jst)]

Opening this up. Is there anything that actually needs fixed here? Seems some cleanup could be done...

Comment 5

[Andrei Purice]

Hello,
Can you still reproduce this issue or should we close it?

Comment 6

[timeless]

@andrei this is embarrassing. It would have taken you seconds to get to the relevant code and see that it has been fixed.

Could you please practice using your tools some more before you ask poor reporters to do work that you should be able to do?

https://searchfox.org/mozilla-central/source/dom/xslt/xpath/txCoreFunctionCall.cpp#190-193

nsresult txCoreFunctionCall::evaluate(txIEvalContext* aContext,
                                      txAExprResult** aResult) {
...
  switch (mType) {
    case COUNT: {
...
    case ID: {
...
    case LAST: {
..
    case LOCAL_NAME:
    case NAME:
    case NAMESPACE_URI: {
...
      switch (mType) {
        case LOCAL_NAME: {
...
        }
        case NAMESPACE_URI: {
...
        }
        case NAME: {
...
          return NS_OK;
        }
        default: {
          MOZ_CRASH("Unexpected mType?!");
        }
      }
      MOZ_CRASH("Inner mType switch should have returned!");
    }
    case POSITION: {
...
    }

I don't appear to have the ability to resolve as fixed.

[e-mail]: won't you please figure out how to mark this as fixed?

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.