On Tue, 17 Dec 2019 at 18:16, Taylor Simpson <tsimp...@quicinc.com> wrote: > Question 1: > I see this error from checkpatch.pl > ERROR: Macros with complex values should be enclosed in parenthesis > However, there are times when the code will not compile with parenthesis. > For example, we have a file that defined all the instruction attributes. > Each line has > DEF_ATTRIB(LOAD, "Loads from memory", "", "") > So, we create an enum of all the possible attributes as follows > enum { > #define DEF_ATTRIB(NAME, ...) A_##NAME, > #include "attribs_def.h" > #undef DEF_ATTRIB > };
checkpatch is often right, but also often wrong, especially for C macros which are in the general case impossible to parse. If the error makes no sense, you can ignore it. > Question 2: > What is the best source of guidance on breaking down support for a new target > into a patch series? Look at how previous ports did it. Also I thought we'd had a subthread on how best to split things up, but maybe I'm misremembering. > I see avr being reviewed currently. I have mostly new files: 12 in > linux-user/hexagon, and ~50 in target/hexagon. I also need to add test cases > and a container for the toolchain. Is it OK to break things down mostly at > file boundaries? No, file boundaries are generally a bad choice of breakdown. You want to split at conceptual boundaries, ie one chunk of functionality that can be comprehended in one go without having to refer forward to other patches. thanks -- PMM