Even though technically I prefer adaptor concept with separate file which is mostly similar to Dave?s for each define case, I?d like to talk the policy how to handle it.
For example, API set addition should be cared as separate module without ifdef in the code. different features in the same API, should be cared as adaptor with separate file. BR, Uze Choi From: Dave Thaler [mailto:[email protected]] Sent: Saturday, November 19, 2016 2:45 PM To: Daniel Mihai; uzchoi at samsung.com; iotivity-dev at lists.iotivity.org Subject: RE: RE: RE: [dev] [suggestion] RE: Change in iotivity[master]: fix findresource failure issue. Dan?s response is close to what was going to say too. Rather than having any ifdefs in the foo1 case, it could instead support a registered callback mechanism that means there?s only one ifdef for the feature in the whole code base, and that?s the ifdef to register one or more callbacks which are implements in a separate file (that is only compiled when that feature is enabled). void foo3() { int a, b, c; // 20 lines of non-optional code that calculate the values of a, b and c if (g_FooExtensionHandler) { g_FooExtensionHandler(a, b, &c); } // 20 lines of non-optional code that is using a, b and c } registerExtensionHandler(ExtensionHandler handler) { g_FooExtensionHandler = handler; } void init() { // ? #ifdef MY_OPTIONAL_FEATURE registerExtensionHandler(MyOptionalFunction); #endif } // Separate file which is only compiled if MY_OPTIONAL_FEATURE is set. void MyOptionalFunction(int a, int b, int *c) { // 100 lines of optional code that is using a, b, c } From: Daniel Mihai Sent: Thursday, November 17, 2016 4:43 PM To: uzchoi at samsung.com; Dave Thaler <dthaler at microsoft.com>; iotivity-dev at lists.iotivity.org Subject: RE: RE: RE: [dev] [suggestion] RE: Change in iotivity[master]: fix findresource failure issue. >> Dan, for the 100 lines of code, do we need them fragmented? What happen we >> enforce them use common code? Example for what I had in mind: void foo1() { int a, b, c; // 20 lines of non-optional code that calculate the values of a, b and c #ifdef MY_OPTIONAL_FEATURE // 100 lines of optional code that is using a, b, c #endif // 20 lines of non-optional code that is using a, b and c } Sometimes it might be preferable to change that to: void foo2() { int a, b, c; // 20 lines of non-optional code that calculate the values of a, b and c #ifdef MY_OPTIONAL_FEATURE MyOptionalFunction(a, b, &c); #endif // 20 lines of non-optional code that is using a, b and c } void MyOptionalFunction(int a, int b, int *c) { // 100 lines of optional code that is using a, b, c } The second version of foo() can be useful, because even without being familiar with the optional feature, I can infer quickly that: - the optional feature will not change the values of a and b - the optional feature might change the value of c Another useful pattern for extension point might be: "observers". Instead of: void bar1() { // 20 lines of non-optional code #ifdef MY_OPTIONAL_FEATURE1 // 100 lines of optional code1 #endif #ifdef MY_OPTIONAL_FEATURE2 // 50 lines of optional code2 #endif // 20 lines of non-optional code } sometimes it might be better to use: void bar2() { // 20 lines of non-optional code NotifyAllObserversThatBarIsRunning(); // 20 lines of non-optional code } I am a kinda reluctant to recommend these Observers though because, depending on their implementation, it might be relatively easy for an attacker to exploit a buffer overrun vulnerability to setup their own Observer... Dan From: ??? [mailto:[email protected]] Sent: Wednesday, November 16, 2016 9:31 PM To: Dave Thaler <dthaler at microsoft.com>; Daniel Mihai <Daniel.Mihai at microsoft.com>; iotivity-dev at lists.iotivity.org Subject: RE: RE: RE: [dev] [suggestion] RE: Change in iotivity[master]: fix findresource failure issue. I understand there is a GAP from me. Anyway, If we couple them together, policy setting is not feasible. BR, Uze Choi --------- Original Message --------- Sender : Dave Thaler <dthaler at microsoft.com> Date : 2016-11-17 14:27 (GMT+9) Title : RE: RE: [dev] [suggestion] RE: Change in iotivity[master]: fix findresource failure issue. I disagree with treating all features the same, as we?ve discussed before. There?s significant differences between things that are certified vs not certified, things that are based on a stable document (e.g. IETF RFC) vs a spec that can change at any time, etc. From: ??? [mailto:[email protected]] Sent: Thursday, November 17, 2016 2:04 PM To: Daniel Mihai <Daniel.Mihai at microsoft.com>; Dave Thaler <dthaler at microsoft.com>; iotivity-dev at lists.iotivity.org Subject: RE: RE: [dev] [suggestion] RE: Change in iotivity[master]: fix findresource failure issue. I don't like the optional and experimental terminology in IoTivity, let's normalize all categories as same. Dan, for the 100 lines of code, do we need them fragmented? What happen we enforce them use common code? BR, Uze Choi --------- Original Message --------- Sender : Daniel Mihai <Daniel.Mihai at microsoft.com> Date : 2016-11-17 03:23 (GMT+9) Title : RE: [dev] [suggestion] RE: Change in iotivity[master]: fix findresource failure issue. Obviously we should try to keep the number, complexity and size of optional/experimental features at a minimum. When we really have to add optional features, we should try harder to add appropriate check-in verification test coverage in Jenkins. For example: we don?t seem to have enough test coverage for SECURED=1. I bet there are other optional features that also don?t have adequate test coverage either. Also, hopefully we can design smaller/better extension points for optional features, and move more of the optional code into separate source code modules, and possibly separate LIBs. Hopefully we can avoid that way most of the time adding ~100 lines of #ifdef code in the middle of a non-optional function, etc. Dan From: iotivity-dev-bounces at lists.iotivity.org [mailto:[email protected]] On Behalf Of Dave Thaler via iotivity-dev Sent: Wednesday, November 16, 2016 1:40 AM To: uzchoi at samsung.com; iotivity-dev at lists.iotivity.org Subject: Re: [dev] [suggestion] RE: Change in iotivity[master]: fix findresource failure issue. In general it?s better to factor code out so there are no ifdefs inline. This obviously takes more work, but is where I think we need to move over time, i.e. to refactor code to remove ifdefs rather than continually adding more. The challenge though is with optional/experimental features which are sometimes harder to factor than say platform-specific ifdefs. From: iotivity-dev-bounces at lists.iotivity.org [mailto:[email protected]] On Behalf Of ??? Sent: Wednesday, November 16, 2016 5:17 PM To: iotivity-dev at lists.iotivity.org Subject: [dev] [suggestion] RE: Change in iotivity[master]: fix findresource failure issue. Hi IoTivity, On the source code, there are lots of #ifdef code, Maintanence and behavior consistency view, it is very risk. Especially, on the common base code, I prefer not to use it as much as possible. Any suggestion to remove this kind of issue? BR, Uze Choi --------- Original Message --------- Sender : Habib Virji (Code Review) <gerrit at iotivity.org> Date : 2016-11-16 17:09 (GMT+9) Title : Change in iotivity[master]: fix findresource failure issue. Habib Virji has posted comments on this change. Change subject: fix findresource failure issue. ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://gerrit.iotivity.org/gerrit/14397 To unsubscribe, visit https://gerrit.iotivity.org/gerrit/settings Gerrit-MessageType: comment Gerrit-Change-Id: I70314bdc6506101b551c9389bb694cad3bf222f5 Gerrit-PatchSet: 1 Gerrit-Project: iotivity Gerrit-Branch: master Gerrit-Owner: jihwan seo <jihwan.seo at samsung.com> Gerrit-Reviewer: Ashok Babu Channa <ashok.channa at samsung.com> Gerrit-Reviewer: Dave Thaler <dthaler at microsoft.com> Gerrit-Reviewer: Habib Virji <habib.virji at samsung.com> Gerrit-Reviewer: JungYong KIM <jyong2.kim at samsung.com> Gerrit-Reviewer: Larry Sachs <larry.j.sachs at intel.com> Gerrit-Reviewer: MyeongGi Jeong <myeong.jeong at samsung.com> Gerrit-Reviewer: Uze Choi <uzchoi at samsung.com> Gerrit-Reviewer: jenkins-iotivity <jenkins-iotivity at opendaylight.org> Gerrit-HasComments: No <http://ext.samsung.net/mail/ext/v1/external/status/update?userid=uzchoi&do=bWFpbElEPTIwMTYxMTE3MDUzMDQ4ZXBjbXMxcDExZTJlZjBmYjc3ZGRlMTBjMjE0N2Q2NDhhZDIzMmY1ZiZyZWNpcGllbnRBZGRyZXNzPURhbmllbC5NaWhhaUBtaWNyb3NvZnQuY29t> -------------- next part -------------- HTML ?????? ??????????????... URL: <http://lists.iotivity.org/pipermail/iotivity-dev/attachments/20161124/6c057389/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: image001.gif Type: image/gif Size: 13402 bytes Desc: ?????? ?? ????????. URL: <http://lists.iotivity.org/pipermail/iotivity-dev/attachments/20161124/6c057389/attachment.gif>
