<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Looks reasonable to me!<div><br></div><div>Thanks,<br><div>
<div style="color: rgb(0, 0, 0); letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div style="font-size: medium; orphans: 2; widows: 2; border-collapse: separate; border-spacing: 0px;"><span style="font-size: 12px; orphans: auto; widows: auto;">Enrico Granata</span><br style="font-size: 12px; orphans: auto; widows: auto;"><span style="font-size: 12px; orphans: auto; widows: auto;">✉ egranata@</span><font color="#ff2600" style="font-size: 12px; orphans: auto; widows: auto;"></font><span style="font-size: 12px; orphans: auto; widows: auto;">.com</span><br style="font-size: 12px; orphans: auto; widows: auto;"><span style="font-size: 12px; orphans: auto; widows: auto;">✆ 27683</span></div></div>
</div>
<br><div><div>On Feb 20, 2013, at 3:55 PM, "Malea, Daniel" <<a href="mailto:daniel.malea@intel.com">daniel.malea@intel.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">Thanks guys for the prompt feedback and explanation. The uninitialized member is worrisome…<br><br>Does the attached fix and test update look OK?<br><br><br>Dan<br><br><span><fix_test_command_script_v2.patch></span><br><br><br>On 2013-02-20, at 6:13 PM, Jim Ingham <<a href="mailto:jingham@apple.com">jingham@apple.com</a>><br> wrote:<br><br><blockquote type="cite">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.<br><br>Jim<br><br>On Feb 20, 2013, at 3:00 PM, Enrico Granata <<a href="mailto:egranata@apple.com">egranata@apple.com</a>> wrote:<br><br><blockquote type="cite">Daniel,<br>I am pretty sure this test case used to pass. However, I did reproduce the failure you see.<br><br>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.<br>If you look at AddUserCommand(), it says:<br><br> // do not allow replacement of internal commands<br> if (CommandExists(name_cstr))<br> {<br> if (can_replace == false)<br> return false;<br> if (m_command_dict[name]->IsRemovable() == false)<br> return false;<br> }<br><br>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.<br>If you look at CommandObjectFrame, it is a MultiwordCommand, and for these:<br><br> virtual bool<br> IsRemovable() const { return m_can_be_removed; }<br><br>The *real* bug seems to be that m_can_be_removed is never initialized when the constructor runs.<br><br>My proposed fix (I have not prepared a patch for it because I am currently busy with a different task):<br>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)<br>b) if necessary, individual multiword commands can change the value of the flag - or third-parties can call SetRemovable() - with the right value<br><br>As a side note, plugin multiword commands already get this right by calling SetRemovable(true) (SBCommandInterpeter.cpp lines 416 and 470)<br><br>I will gladly appreciate any feedback on this proposal.<br>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.<br><br>Thanks,<br>Enrico Granata<br>✉ egranata@.com<br>✆ 27683<br><br>On Feb 20, 2013, at 2:36 PM, "Malea, Daniel" <<a href="mailto:daniel.malea@intel.com">daniel.malea@intel.com</a>> wrote:<br><br><blockquote type="cite">Hi,<br><br>I noticed the TestCommandScript.py test case is failing one line 118, where it verifies that a user cannot redefine a command named 'frame'.<br><br>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.<br><br>Attached patch should fix it. Any concerns/comments, or can I commit this as-is?<br><br>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.<br><br><br>Dan<br><br><br><fix_test_command_script.patch><br><br></blockquote><br>_______________________________________________<br>lldb-commits mailing list<br><a href="mailto:lldb-commits@cs.uiuc.edu">lldb-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits<br></blockquote><br></blockquote><br></blockquote></div><br></div></body></html>