search

intel / gmmlib

Other
158 stars 86 forks source link

Fix overlapping comparison warning #88

Closed edwarddavidbaker closed 3 years ago

edwarddavidbaker commented 3 years ago

NumSamples can not be 8 and 16 at the same time. Closes #87.

warning: overlapping comparisons always evaluate to false [-Wtautological-overlap-compare]
Source/GmmLib/Texture/GmmGen9Texture.cpp:595:46:
           ((pTexInfo->MSAA.NumSamples == 8) && (pTexInfo->MSAA.NumSamples == 16)) &&
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
johnmach commented 3 years ago

Thanks Edward. Might have overwritten. Will update this in the repo.

edwarddavidbaker commented 3 years ago

Thanks John! Fixed in https://github.com/intel/gmmlib/commit/d84606bd4bb603119c15e01e24af9920e27ef704 .

paulmenzel commented 2 years ago

@johnmach, out of curiosity, why didn’t you merge this merge/pull request, and instead authored the commit d84606bd (Fix Overlapping Comparision)](https://github.com/intel/gmmlib/issues/66) yourself, which also seems to reference the wrong issue (#66 instead of #87) and misses a commit message body, @edwarddavidbaker’s commit in this merge/pull request has?

johnmach commented 2 years ago

@paulmenzel

Thanks for raising this.

  1. out of curiosity, why didn’t you merge this merge/pull request, and instead authored the commit https://github.com/intel/gmmlib/commit/d84606bd4bb603119c15e01e24af9920e27ef704 (Fix Overlapping Comparision) yourself

    Gmmlib follows a standard process of bringing in changes to opensource https://github.com/intel/gmmlib. Firstly, for any external PR/issues the fix is merged in internal repo and then after certain test cycles and validation it is promoted to open source. At the same time the corresponding external PR/issues is closed referencing the fix commit. I see in above closed PR, edwarddavidbaker closed this referencing to the fix commit https://github.com/intel/gmmlib/commit/d84606bd4bb603119c15e01e24af9920e27ef704.

  2. which also seems to reference the wrong issue (#66 instead of #87).

    I agree. This is referencing the wrong issue. This is due to the internal commit pull request i.e. X appended to the commit title in internal repo. The same commit flows to the open source repo for which the github renders it has #X referencing to external repo space. Hence, points out to wrong reference. We look forward to resolve this.

  3. and misses a commit message body, @edwarddavidbaker’s commit in this merge/pull request has

    We would really like to take this suggestion going forward. We will try to incorporate all commit details of external PR/issue in our internal merges.