☢️
Created attachment 430064 [details] Patch
Committed r278237 (238274@main): <https://commits.webkit.org/238274@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 430064 [details].
<rdar://problem/78646851>
Comment on attachment 430064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430064&action=review > Source/JavaScriptCore/jit/JITSizeStatistics.h:31 > +#include "CCallHelpers.h" This is the wrong thing to do because this header file doesn't really depend on CCallHelpers.h. The right thing to do is to #include "CCallHelpers.h" in the files that actually uses JITSizeStatistics::markStart and markEnd. AFAICT, the only files that need it are JIT.cpp and DFGSpeculativeJIT.cpp, and those files already #include CCallHelpers.h indirectly. So, what is motivating this change?
(In reply to Mark Lam from comment #4) > Comment on attachment 430064 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=430064&action=review > > > Source/JavaScriptCore/jit/JITSizeStatistics.h:31 > > +#include "CCallHelpers.h" > > This is the wrong thing to do because this header file doesn't really depend > on CCallHelpers.h. > > The right thing to do is to #include "CCallHelpers.h" in the files that > actually uses JITSizeStatistics::markStart and markEnd. AFAICT, the only > files that need it are JIT.cpp and DFGSpeculativeJIT.cpp, and those files > already #include CCallHelpers.h indirectly. So, what is motivating this > change? In JITSizeStatistics.h there is an use of the CCallHelpers::Label type, which is defined all the way up the class inheritance chain in the AbstractMacroAssembler<T>. I gave it a small spin at trying to figure out the correct “T” to use but it ended up needing to know the particular type for the current target architecture and anyway needed including headers that CCallHelpers.h itself is already including… in the end it seemed easier and much more readable to keep the CCallHelpers::Label usage as-is and include CCallHelper.h If anybody has a better suggestion on how to get a definition for the CCallHelpers::Label type I can try to give it a try.
(In reply to Adrian Perez from comment #5) > (In reply to Mark Lam from comment #4) > > Comment on attachment 430064 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=430064&action=review > > > > > Source/JavaScriptCore/jit/JITSizeStatistics.h:31 > > > +#include "CCallHelpers.h" > > > > This is the wrong thing to do because this header file doesn't really depend > > on CCallHelpers.h. > > > > The right thing to do is to #include "CCallHelpers.h" in the files that > > actually uses JITSizeStatistics::markStart and markEnd. AFAICT, the only > > files that need it are JIT.cpp and DFGSpeculativeJIT.cpp, and those files > > already #include CCallHelpers.h indirectly. So, what is motivating this > > change? > > In JITSizeStatistics.h there is an use of the CCallHelpers::Label > type, which is defined all the way up the class inheritance chain > in the AbstractMacroAssembler<T>. I gave it a small spin at trying > to figure out the correct “T” to use but it ended up needing to know > the particular type for the current target architecture and anyway > needed including headers that CCallHelpers.h itself is already > including… in the end it seemed easier and much more readable to > keep the CCallHelpers::Label usage as-is and include CCallHelper.h > > If anybody has a better suggestion on how to get a definition for > the CCallHelpers::Label type I can try to give it a try. I stand corrected. That's unfortunate. Thanks for explaining. We could have just used MacroAssembler::Label, but then, we'd still be #include'ing MacroAssembler.h. Given this, it's not worth thinking more about this issue. Your fix is correct. Sorry for the noise.
(In reply to Mark Lam from comment #6) > (In reply to Adrian Perez from comment #5) > > (In reply to Mark Lam from comment #4) > > > Comment on attachment 430064 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=430064&action=review > > > > > > > Source/JavaScriptCore/jit/JITSizeStatistics.h:31 > > > > +#include "CCallHelpers.h" > > > > > > This is the wrong thing to do because this header file doesn't really depend > > > on CCallHelpers.h. > > > > > > The right thing to do is to #include "CCallHelpers.h" in the files that > > > actually uses JITSizeStatistics::markStart and markEnd. AFAICT, the only > > > files that need it are JIT.cpp and DFGSpeculativeJIT.cpp, and those files > > > already #include CCallHelpers.h indirectly. So, what is motivating this > > > change? > > > > In JITSizeStatistics.h there is an use of the CCallHelpers::Label > > type, which is defined all the way up the class inheritance chain > > in the AbstractMacroAssembler<T>. I gave it a small spin at trying > > to figure out the correct “T” to use but it ended up needing to know > > the particular type for the current target architecture and anyway > > needed including headers that CCallHelpers.h itself is already > > including… in the end it seemed easier and much more readable to > > keep the CCallHelpers::Label usage as-is and include CCallHelper.h > > > > If anybody has a better suggestion on how to get a definition for > > the CCallHelpers::Label type I can try to give it a try. > > I stand corrected. That's unfortunate. Thanks for explaining. We could > have just used MacroAssembler::Label, but then, we'd still be #include'ing > MacroAssembler.h. Given this, it's not worth thinking more about this > issue. Your fix is correct. Sorry for the noise. No problem at all, that's what we are here for—making WebKit as good as we can :)