On 2013-02-20, at 6:13 PM, Jim Ingham wrote: > Ah, I understand now. Yeah, the API is a little weird because you are passing a flag saying "Let me replace an extant command if the extant command agrees to be replaced." which seems a little over-specified. But given that, Enrico is right and I was misreading the code. > > Jim > > On Feb 20, 2013, at 3:00 PM, Enrico Granata wrote: > >> Daniel, >> I am pretty sure this test case used to pass. However, I did reproduce the failure you see. >> >> As for fixing it, I am not sure passing "false" there would be the right thing. Doing so would make it impossible for users to replace their own scripted commands. >> If you look at AddUserCommand(), it says: >> >> // do not allow replacement of internal commands >> if (CommandExists(name_cstr)) >> { >> if (can_replace == false) >> return false; >> if (m_command_dict[name]->IsRemovable() == false) >> return false; >> } >> >> Regardless of can_replace, a command can explicitly mark itself as non-removable, and that is the way system commands are supposed to be marked. >> If you look at CommandObjectFrame, it is a MultiwordCommand, and for these: >> >> virtual bool >> IsRemovable() const { return m_can_be_removed; } >> >> The *real* bug seems to be that m_can_be_removed is never initialized when the constructor runs. >> >> My proposed fix (I have not prepared a patch for it because I am currently busy with a different task): >> a) the constructor for CommandObjectMultiword should initialize m_can_be_removed - my guess is that the default value should be false (because that is also the default for CommandObject) >> b) if necessary, individual multiword commands can change the value of the flag - or third-parties can call SetRemovable() - with the right value >> >> As a side note, plugin multiword commands already get this right by calling SetRemovable(true) (SBCommandInterpeter.cpp lines 416 and 470) >> >> I will gladly appreciate any feedback on this proposal. >> Assuming there are no objections, Daniel, feel free to write a patch to this effect, or if you are already busy, let me know and I will do this ASAP. >> >> Thanks, >> Enrico Granata >> ✉ egranata@.com >> ✆ 27683 >> >> On Feb 20, 2013, at 2:36 PM, "Malea, Daniel" wrote: >> >>> Hi, >>> >>> I noticed the TestCommandScript.py test case is failing one line 118, where it verifies that a user cannot redefine a command named 'frame'. >>> >>> I believe the failure is due to CommandObjectCommands.cpp passing the wrong value (true) for the 'can_replace' parameter to CommandInterpreter::AddUserCommand… When this is set to 'true', which allows the test-case to re-define the 'frame' command to be something else. >>> >>> Attached patch should fix it. Any concerns/comments, or can I commit this as-is? >>> >>> I'm still not exactly sure how CommandObjectCommands works, and why there's two code paths that end up calling AddUserCommand..so I figured I'd run it by the list first. >>> >>> >>> Dan >>> >>> >>> >>> >> >> _______________________________________________ >> lldb-commits mailing list >> lldb-commits@cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits >