davrec added a comment. This part of the description is confusing:
> We take two types, X and Y, which we wish to unify as input. > These types must have the same (qualified or unqualified) canonical > type. > > We dive down fast through top-level type sugar nodes, to the > underlying canonical node. If these canonical nodes differ, we > build a common one out of the two, unifying any sugar they had. > Note that this might involve a recursive call to unify any children > of those. We then return that canonical node, handling any qualifiers. > > If they don't differ, we walk up the list of sugar type nodes we dived > through, finding the last identical pair, and returning that as the > result, again handling qualifiers. The first paragraph says X and Y must have the same canonical type, the second suggests they need not. In all the test cases, they have the same canonical type. IIUC the second paragraph only applies to the stuff done in D130308 <https://reviews.llvm.org/D130308>; is that correct? If so, the description should clearly state this patch only handles the case when the canonical types of X and Y are identical, and note that if the canonical types of X and Y differ, this patch does nothing, but D130308 <https://reviews.llvm.org/D130308> adds some additional logic to search for common sugar in child type nodes of X and Y to use in the merged type. And of course please add a full description to D130308 <https://reviews.llvm.org/D130308> as well when you have a chance. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111283/new/ https://reviews.llvm.org/D111283 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
