<div dir="ltr">Please avoid directly replying to the email as they won't be logged in the phabricator.<div><br></div><div>This boolean is logged into YAML output, while I don't have data, I could bet there will be users depending on it even if it looks so unlikely. We don't own the tool in the first place, in that case, I felt if there is a bug in its constructor, we should fix the bug instead of abruptly deleting it, even if it looks so easy to be derived from other values. </div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Sep 26, 2022 at 3:14 PM Brett Wilson <<a href="mailto:brettw@gmail.com">brettw@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Mon, Sep 26, 2022 at 1:42 PM Haowei Wu via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">haowei added a comment.<br>
<br>
@brettw Thanks for the clarification. I was oncall for buildgardener last week and got quite busy due to breakages.<br>
<br>
Thanks for your clarification, that is really helpful to understand the your intent. While I still don't see the way these constructors are implemented is an issue. I don't have a strong opinions on this and OK with your refactors.<br>
<br>
"semantically equivalent" means these test code should have same input/output, performance same routines of task and use same amount of memory. Before your change, these typeinfo's symbolID in the test are always pointing to a constant EmptySID, after your change, some of these typeinfo points to an empty symbol ID in allocated in memory when typeinfo is created. While the output is the same, they are not equivalent. For refactor changes, I would prefer to avoid introducing such changes.<br>
<br>
I have one question about deleting the IsInGlobalNamespace. I understand it is not well implemented and has a bug in its constructor, but I don't think we should simply delete it just because it is buggy and not well used. It is part of C++ and commonly used. Do you have any plan to add it back after all the refactor work finishes?<br></blockquote><div><br></div><div>I guess I'll ask you: what does this boolean mean and why do we need it? This flag was only ever derived from the "path" which is just a member in the same class. Users already have the full namespace information as far as I can tell.</div><div><br></div><div>Brett</div></div></div>
</blockquote></div>