[GitHub] [xerces-c] rleigh-codelibre commented on a change in pull request #2: Commit fuzzer source code.

[GitBox <gi...@apache.org>]

rleigh-codelibre commented on a change in pull request #2:
URL: https://github.com/apache/xerces-c/pull/2#discussion_r443126284



##########
File path: .gitignore
##########
@@ -105,3 +105,6 @@ tests/scripts/*.log
 tests/scripts/*.trs
 tests/test-suite.log
 Testing/
+
+# Vincent
+build/

Review comment:
       This change should be kept private.  Please could you remove it from the PR?

##########
File path: cmake/XercesIntTypes.cmake
##########
@@ -58,14 +58,14 @@ set(HAVE_OFF_T ${SIZEOF_OFF_T})
 set(HAVE_SIZE_T ${SIZEOF_SIZE_T})
 set(HAVE_SSIZE_T ${SSIZEOF_SSIZE_T})
 set(HAVE_WCHAR_T ${WCHAROF_WCHAR_T})
-if(SIZEOF_SIZE_T)
+if(HAVE_SIZEOF_SIZE_T)

Review comment:
       I'm not sure where these changes have come from.  Is this a reversion of an earlier change?
   
   If the history is messy, it might be easiest to rebase all your changes onto the current `master` and start a new PR.

##########
File path: CMakeLists.txt
##########
@@ -177,8 +177,14 @@ install(
 # Process subdirectories
 add_subdirectory(doc)
 add_subdirectory(src)
-add_subdirectory(tests)
-add_subdirectory(samples)
+if (NOT (DEFINED XERCES_BUILD_FUZZERS))

Review comment:
       Why are these specifically excluded?  It might be nicer to only use the fuzzer-specific conditions on the fuzzer code itself.

##########
File path: build_fuzzer.sh
##########
@@ -0,0 +1,8 @@
+#!/bin/bash

Review comment:
       I don't see this file being used anywhere.  Does it really belong in the source tree?
   
   If it does belong, could it go into the `scripts` subdirectory alongside the CI scripts?

##########
File path: src/xercesc/util/XMLChar.cpp
##########
@@ -8837,7 +8837,7 @@ XMLByte XMLChar1_1::fgCharCharsTable1_1[0x10000] =
 
 #include <stdio.h>
 
-static XMLCh gTmpCharTable[0xFFFF];

Review comment:
       I'm unsure about these any other code changes, above.  Are they intentional?
   
   If they are, please could you open separate PRs for them so we can keep code fixes separate from the fuzzer addition.

##########
File path: CMakeLists.txt
##########
@@ -177,8 +177,14 @@ install(
 # Process subdirectories
 add_subdirectory(doc)
 add_subdirectory(src)
-add_subdirectory(tests)
-add_subdirectory(samples)
+if (NOT (DEFINED XERCES_BUILD_FUZZERS))
+  add_subdirectory(tests)
+  add_subdirectory(samples)
+endif()
+
+if ((DEFINED XERCES_BUILD_FUZZERS) OR (DEFINED XERCES_BUILD_FOR_OSS_FUZZ))

Review comment:
       Additionally, rather than use XERCES_BUILD_FUZZERS, why not define an option `fuzzers` with `option()` defaulting to off, then you can just use `-Dfuzzers=ON`?

##########
File path: CMakeLists.txt
##########
@@ -177,8 +177,14 @@ install(
 # Process subdirectories
 add_subdirectory(doc)
 add_subdirectory(src)
-add_subdirectory(tests)
-add_subdirectory(samples)
+if (NOT (DEFINED XERCES_BUILD_FUZZERS))
+  add_subdirectory(tests)
+  add_subdirectory(samples)
+endif()
+
+if ((DEFINED XERCES_BUILD_FUZZERS) OR (DEFINED XERCES_BUILD_FOR_OSS_FUZZ))

Review comment:
       Please always include all subdirectories, and place the above conditionals around the logic inside the file.  This ensures the logic is evaluated whatever the cmake options, and helps to prevent regressions.

##########
File path: fuzzers/CMakeLists.txt
##########
@@ -0,0 +1,90 @@
+# CMake build for xerces-c
+#
+# Written by Roger Leigh <rl...@codelibre.net>
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Definitions required for building
+add_definitions(
+  -DHAVE_CONFIG_H=1

Review comment:
       If this is necessary, I think target_compile_definitions would be more appropriate, rather than setting it globally.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: c-dev-unsubscribe@xerces.apache.org
For additional commands, e-mail: c-dev-help@xerces.apache.org